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

fix DateOffset eq to depend on normalize attr #21404

Merged
merged 5 commits into from
Jun 13, 2018
Merged

fix DateOffset eq to depend on normalize attr #21404

merged 5 commits into from
Jun 13, 2018

Conversation

jbrockmendel
Copy link
Member

This has a bullet point in #18854 but doesn't appear to have its own issue.

Current:

now = pd.Timestamp.now()
off = pd.offsets.Week(n=1, normalize=False)
normed = pd.offsets.Week(n=1, normalize=True)

>>> now + off == now + normed
False
>>> off == normed  # PR changes this comparison to False
True
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

jbrockmendel
jbrockmendel commented Jun 10, 2018
View reviewed changes
pandas/tests/tseries/offsets/test_offsets.py Outdated
# GH#???? changed __eq__ to return False when `normalize` doesnt match
offset = self._offset()
offset2 = self._offset(normalize=True)
assert offset != offset2
Copy link
Member Author

Choose a reason for hiding this comment

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

After substituting in self._offset for BDay, this test is identical to 5 others in this file. I'm putting together a separate PR to try to de-duplicate and parametrize a bunch of things like this.

@codecov
Copy link

codecov bot commented Jun 10, 2018 •
edited
Loading

Codecov Report

Merging #21404 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #21404   +/-   ##
=======================================
  Coverage    91.9%    91.9%           
=======================================
  Files         153      153           
  Lines       49606    49606           
=======================================
  Hits        45589    45589           
  Misses       4017     4017
Flag Coverage Δ
#multiple 90.3% <100%> (ø) ⬆️
#single 41.89% <100%> (ø) ⬆️
Impacted Files Coverage Δ
pandas/tseries/offsets.py 97.24% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 576d5c6...b902cb1. Read the comment docs.

@gfyoung
Copy link
Member

gfyoung commented Jun 10, 2018 •
edited
Loading

This has a bullet point in #18854 but doesn't appear to have its own issue.

@jbrockmendel : How about this one here: #17689? It might be a little familiar to you. 😉

@gfyoung gfyoung added Timedelta Timedelta data type Bug labels Jun 10, 2018
gfyoung
gfyoung approved these changes Jun 10, 2018
View reviewed changes
Copy link
Member

@gfyoung gfyoung left a comment

Choose a reason for hiding this comment

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

This looks good to me!

cc @jreback

@jreback jreback added this to the 0.24.0 milestone Jun 12, 2018
jreback
jreback requested changes Jun 12, 2018
View reviewed changes
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.

looks good. minor comment. ping on green.

doc/source/whatsnew/v0.24.0.txt Outdated
@@ -91,7 +91,7 @@ Categorical
Datetimelike
^^^^^^^^^^^^

-
- Fixed Bug where two :class:`DateOffset` objects with different `normalize` attributes could evaluate as equal (:issue:`21404`)
Copy link
Contributor

Choose a reason for hiding this comment

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

double backticks on normalize

@gfyoung
Copy link
Member

gfyoung commented Jun 12, 2018

@jreback : Everything is green here.

@jreback
Copy link
Contributor

jreback commented Jun 13, 2018

I merged something which now causes a conflict here. @jbrockmendel can you rebase. @gfyoung can merge on green.

@gfyoung gfyoung merged commit b5fc769 into pandas-dev:master Jun 13, 2018
@gfyoung
Copy link
Member

gfyoung commented Jun 13, 2018

Thanks @jbrockmendel !

This was referenced Jun 13, 2018
Closed
Closed
david-liu-brattle-1 pushed a commit to david-liu-brattle-1/pandas that referenced this pull request Jun 18, 2018
@jbrockmendel @david-liu-brattle-1
BUG: Fix DateOffset eq to depend on normalize attr (pandas-dev#21404)
c18b163 
Partially addresses pandas-devgh-18854
david-liu-brattle-1 pushed a commit to david-liu-brattle-1/pandas that referenced this pull request Jun 19, 2018
@jbrockmendel @david-liu-brattle-1
BUG: Fix DateOffset eq to depend on normalize attr (pandas-dev#21404)
7ab0e80 
Partially addresses pandas-devgh-18854
@jbrockmendel jbrockmendel mentioned this pull request Jun 21, 2018
Closed
59 tasks
@jbrockmendel jbrockmendel deleted the cmp_normalize branch June 22, 2018 03:27
Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018
@jbrockmendel
BUG: Fix DateOffset eq to depend on normalize attr (pandas-dev#21404)
bc4ce26 
Partially addresses pandas-devgh-18854
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jreback jreback jreback approved these changes

@gfyoung gfyoung gfyoung approved these changes

Assignees
No one assigned
Labels
Bug Timedelta Timedelta data type
Projects
None yet
Milestone
0.24.0
Development

Successfully merging this pull request may close these issues.
3 participants
@jbrockmendel @gfyoung @jreback