Skip to content

Regression in offsets caused offsets to be no longer hashable #37288

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

Merged
merged 8 commits into from
Oct 22, 2020

Conversation

phofl
Copy link
Member

@phofl phofl commented Oct 20, 2020

  • closes BUG: offsets are now unhashable #37267
  • tests added / passed Not quite sure, if these test is sufficient, if yes, I would tests for other functions.
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

@phofl phofl changed the title 37267 Regression in offsets caused offsets to be no longer hashable Oct 20, 2020
@phofl phofl added Datetime Datetime data dtype Regression Functionality that used to work in a prior pandas version labels Oct 20, 2020
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

also can you run an asv to make sure this doesn't change perf

@jreback jreback added this to the 1.1.4 milestone Oct 20, 2020
@jreback jreback added the Frequency DateOffsets label Oct 20, 2020
@jreback
Copy link
Contributor

jreback commented Oct 20, 2020

great code change looks good. if you'd report on the asv's we can merge.

@phofl
Copy link
Member Author

phofl commented Oct 20, 2020

Thanks, benchmark has just finished. No changes

BENCHMARKS NOT SIGNIFICANTLY CHANGED.

@@ -752,6 +752,9 @@ cdef class Tick(SingleConstructorOffset):
"Tick offset with `normalize=True` are not allowed."
)

def __hash__(self) -> int:
Copy link
Contributor

Choose a reason for hiding this comment

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

@jbrockmendel ok here?

Copy link
Member

Choose a reason for hiding this comment

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

yep, LGTM

Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!
Added a few comments

Co-authored-by: Joris Van den Bossche <[email protected]>
@jreback jreback merged commit 038aab9 into pandas-dev:master Oct 22, 2020
@jreback
Copy link
Contributor

jreback commented Oct 22, 2020

thanks @phofl nice

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Datetime Datetime data dtype Frequency DateOffsets Regression Functionality that used to work in a prior pandas version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: offsets are now unhashable
4 participants