Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

BUG: DatetimeIndex.diff raising TypeError #55761

Merged
merged 4 commits into from
Oct 31, 2023
Merged

BUG: DatetimeIndex.diff raising TypeError #55761

merged 4 commits into from
Oct 31, 2023

Conversation

lukemanley
Copy link
Member

@lukemanley lukemanley added the Regression Functionality that used to work in a prior pandas version label Oct 30, 2023
@lukemanley lukemanley added this to the 2.2 milestone Oct 30, 2023
@lukemanley lukemanley changed the title BUG: DatetimeIndex.diff raising TypeError REGR: DatetimeIndex.diff raising TypeError Oct 30, 2023
@lukemanley lukemanley modified the milestones: 2.2, 2.1.3 Oct 30, 2023
@pierre-haessig pierre-haessig mentioned this pull request Oct 30, 2023
Closed
3 tasks
jbrockmendel
jbrockmendel reviewed Oct 30, 2023
View reviewed changes
pandas/core/indexes/base.py Outdated
diff = self.to_series().diff(periods)
if diff.dtype != self.dtype:
# e.g. DTI.diff -> TDI
return Index(diff)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's just return Index(diff) unconditionally?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should use self._constructor in order to preserve subclasses as much as possible?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are there any cases where it makes a difference?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

class MyIndex(Index):
    def _constructor(self):
        return MyIndex

>>> MyIndex([1, 2]).diff(MyIndex([1, 3]))

Wouldn't it be a regression if we didn't return a MyIndex here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we dont support 3rd part Index subclasses

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is noted in the docs that Index subclasses are not supported. @mroeschke, any objections to updating based on that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay I didn't know we didn't support Index subclasses. Fine to update to @jbrockmendel suggestion

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated to return Index(diff) unconditionally

@lukemanley lukemanley added Bug and removed Regression Functionality that used to work in a prior pandas version labels Oct 31, 2023
@lukemanley lukemanley changed the title REGR: DatetimeIndex.diff raising TypeError BUG: DatetimeIndex.diff raising TypeError Oct 31, 2023
@lukemanley
Copy link
Member Author

Reclassified from regression to bug since the Index.diff method was only recently added in #52551 and it looks like this has been the behavior since it was added.

@lukemanley lukemanley added the Index Related to the Index class or subclasses label Oct 31, 2023
@mroeschke mroeschke merged commit f04da3c into pandas-dev:main Oct 31, 2023
@mroeschke
Copy link
Member

Thanks @lukemanley

@lumberbot-app Lumberbot (App)
Copy link

lumberbot-app bot commented Oct 31, 2023

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout 2.1.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 f04da3c1c9f5fd81ce7a04e55a965e4021de2ae9
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #55761: BUG: DatetimeIndex.diff raising TypeError'
  1. Push to a named branch:
git push YOURFORK 2.1.x:auto-backport-of-pr-55761-on-2.1.x
  1. Create a PR against branch 2.1.x, I would have named this PR:

"Backport PR #55761 on branch 2.1.x (BUG: DatetimeIndex.diff raising TypeError)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

lukemanley added a commit to lukemanley/pandas that referenced this pull request Nov 4, 2023
@lukemanley
BUG: DatetimeIndex.diff raising TypeError (pandas-dev#55761)
e09b84e 
* BUG: DatetimeIndex.diff raising TypeError

* add test and whatsnew

* typing

* use Index

(cherry picked from commit f04da3c)
mroeschke pushed a commit that referenced this pull request Nov 6, 2023
@lukemanley
Backport PR #55761 on branch 2.1.x (BUG: DatetimeIndex.diff raising T…
0bf70c7 
…ypeError) (#55819)

* Backport PR #55473: TST: sort index with sliced MultiIndex

* BUG: DatetimeIndex.diff raising TypeError (#55761)

* BUG: DatetimeIndex.diff raising TypeError

* add test and whatsnew

* typing

* use Index

(cherry picked from commit f04da3c)

* fix whatsnew
@lukemanley lukemanley deleted the dti-diff branch November 16, 2023 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbrockmendel jbrockmendel jbrockmendel left review comments

@mroeschke mroeschke mroeschke approved these changes

Assignees
No one assigned
Labels
Bug Index Related to the Index class or subclasses
Projects
None yet
Milestone
2.1.3
3 participants
@lukemanley @mroeschke @jbrockmendel