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

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

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 Timeseries 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

pandas/tests/tseries/offsets/test_ticks.py Outdated Show resolved Hide resolved
@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

pandas/tests/tseries/offsets/test_offsets.py Outdated Show resolved Hide resolved
pandas/_libs/tslibs/offsets.pyx Outdated Show resolved Hide resolved
doc/source/whatsnew/v1.1.4.rst Outdated Show resolved Hide resolved
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
@jreback jreback merged commit 038aab9 into pandas-dev:master Oct 22, 2020
@jreback
Copy link
Contributor

jreback commented Oct 22, 2020

thanks @phofl nice

meeseeksmachine pushed a commit to meeseeksmachine/pandas that referenced this pull request Oct 22, 2020
jorisvandenbossche pushed a commit that referenced this pull request Oct 22, 2020
…ger hashable (#37326)

Co-authored-by: patrick <61934744+phofl@users.noreply.github.com>
@phofl phofl deleted the 37267 branch October 22, 2020 21:42
kesmit13 pushed a commit to kesmit13/pandas that referenced this pull request Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Frequency DateOffsets Regression Functionality that used to work in a prior pandas version Timeseries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: offsets are now unhashable
4 participants