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.

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

BUG: Behaviour change in 1.5.0 when using Timedelta as Enum data type #49579

Merged
merged 6 commits into from Nov 19, 2022

Conversation

krasch
Copy link
Contributor

@krasch krasch commented Nov 8, 2022

@jbrockmendel
Copy link
Member

pls add tests where appropriate

@krasch
Copy link
Contributor Author

krasch commented Nov 14, 2022

pls add tests where appropriate

done

@jbrockmendel jbrockmendel added this to the 1.5.2 milestone Nov 15, 2022
@@ -1829,6 +1829,7 @@ class Timedelta(_Timedelta):
return NaT

return _timedelta_from_value_and_reso(
Timedelta,
Copy link
Member

Choose a reason for hiding this comment

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

should this be type(self)?

Copy link
Member

Choose a reason for hiding this comment

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

same for other arithmetic methods above

Copy link
Contributor Author

@krasch krasch Nov 15, 2022

Choose a reason for hiding this comment

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

I had added a question re: this but somehow github ate it, weird..

Anyway, here is my concern, if I make the change you are suggesting

td1 = MyTimedeltaSubclass("10 minutes")
td2 = Timedelta ("10 minutes")

then result types will be inconsistent
td1 + td2 -> MyTimedeltaSubclass
td2 + td1 -> Timedelta

Maybe that is fine, but it also kinda weirded me out, which is why I wanted to ask you. It feels a little bit like it should be the responsibility of the subclass to implement the custom __add__ behaviour.

Wondering: does the rest of pandas respect subclasses in this way?

Copy link
Member

Choose a reason for hiding this comment

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

does the rest of pandas respect subclasses in this way?

i dont think we're very consistent about it. IIRC the stdlib timedelta has something like:

def __add__(self, other):
    if self_is_timedelta and other_is_timedelta_subclass:
        return NotImplemented
    [...]

which we could emulate. let's stick a pin in that for now and can keep Timedelta like you currently have

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I go ahead and create an issue for this?

@jbrockmendel
Copy link
Member

Can you add a note in the 1.5.2 file

@krasch krasch marked this pull request as ready for review November 16, 2022 12:32
@krasch krasch changed the title [WIP] Fix Behaviour change in 1.5.0 when using Timedelta as Enum data type BUG: Behaviour change in 1.5.0 when using Timedelta as Enum data type Nov 16, 2022
@mroeschke mroeschke added Regression Functionality that used to work in a prior pandas version Timedelta Timedelta data type labels Nov 16, 2022
Copy link
Member

@mroeschke mroeschke left a comment

Choose a reason for hiding this comment

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

Looks reasonable. @jbrockmendel merge when ready

@jbrockmendel jbrockmendel merged commit 606499d into pandas-dev:main Nov 19, 2022
46 checks passed
@jbrockmendel
Copy link
Member

thanks @krasch

@meeseeksdev
Copy link

meeseeksdev bot commented Nov 19, 2022

Oops, something went wrong applying the patch ... Please have a look at my logs.

phofl pushed a commit to phofl/pandas that referenced this pull request Nov 19, 2022
…pandas-dev#49579)

* Add cls argument to _timedelta_from_value_and_reso

* Add test

* Add fix to changelog

Co-authored-by: krasch <dev@krasch.io>
(cherry picked from commit 606499d)

# Conflicts:
#	pandas/_libs/tslibs/timedeltas.pyx
#	pandas/tests/scalar/timedelta/test_constructors.py
phofl added a commit that referenced this pull request Nov 19, 2022
…en using Timedelta as Enum data type) (#49787)

BUG: Behaviour change in 1.5.0 when using Timedelta as Enum data type (#49579)

Co-authored-by: krasch <dev@krasch.io>
(cherry picked from commit 606499d)
Co-authored-by: krasch <krasch@users.noreply.github.com>
mliu08 pushed a commit to mliu08/pandas that referenced this pull request Nov 27, 2022
…pandas-dev#49579)

* Add cls argument to _timedelta_from_value_and_reso

* Add test

* Add fix to changelog

Co-authored-by: krasch <dev@krasch.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Regression Functionality that used to work in a prior pandas version Timedelta Timedelta data type
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Behaviour change in 1.5.0 when using Timedelta as Enum data type
3 participants