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

CLN: remove tseries.util.pivot_annual/isleapyear #18370

Merged
merged 2 commits into from
Nov 22, 2017
Merged

CLN: remove tseries.util.pivot_annual/isleapyear #18370

merged 2 commits into from
Nov 22, 2017

Conversation

topper-123
Copy link
Contributor

pivot_annual and isleapyear in tseries.util were deprecated in v0.19.

This PR removes them from the code base.

jreback
jreback requested changes Nov 19, 2017
View reviewed changes
pandas/tests/reshape/test_pivot.py

grouped = ts_hourly.groupby(ts_hourly.index.year)
hoy = grouped.apply(lambda x: x.reset_index(drop=True))
hoy = hoy.index.droplevel(0).values
Copy link
Contributor

Choose a reason for hiding this comment

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

anything worthy of a test for these? eg maybe see if something similar using pivot_table (if not can u construct a test)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A point is that pivot_table requires dataframes, and pivot_annual works on series [that need to have a DatetimeIndex]. So pivot_table is strictly speaking not doing the same as pivot_annual.

A deeper second issue is that pivot_annual dayofyear number 60 is always febr. 29, while for other pandas date functionality day 60 switches between febr. 29 and 1.mars , depending on whether we're not having a leap year or not.

I'll take a look into if something can be moved. However, do you not think that such date functionality is checked other places, i..e. we duplicate datetime tests by adding them to TestPivotTable? See for example TestPivotTable.test_pivot_timegrouper, line 716 for some date pivot_table tests.

@jreback jreback added the Deprecate Functionality to remove in pandas label Nov 19, 2017
@jreback jreback mentioned this pull request Nov 19, 2017
Closed
@topper-123 topper-123 changed the title WIP: remove tseries.util.pivot_annual/isleapyear CLN: remove tseries.util.pivot_annual/isleapyear Nov 19, 2017
@pep8speaks
Copy link

pep8speaks commented Nov 20, 2017 •
edited
Loading

Hello @topper-123! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on November 21, 2017 at 21:05 Hours UTC

@codecov
Copy link

codecov bot commented Nov 20, 2017

Codecov Report

Merging #18370 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18370      +/-   ##
==========================================
- Coverage   91.38%   91.36%   -0.03%     
==========================================
  Files         164      163       -1     
  Lines       49790    49753      -37     
==========================================
- Hits        45501    45455      -46     
- Misses       4289     4298       +9
Flag Coverage Δ
#multiple 89.16% <ø> (-0.01%) ⬇️
#single 39.57% <ø> (-0.02%) ⬇️
Impacted Files Coverage Δ
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/core/frame.py 97.8% <0%> (-0.1%) ⬇️
pandas/tseries/offsets.py 96.92% <0%> (+0.01%) ⬆️

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 b00e62c...38ed067. Read the comment docs.

@codecov
Copy link

codecov bot commented Nov 20, 2017 •
edited
Loading

Codecov Report

Merging #18370 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18370      +/-   ##
==========================================
- Coverage   91.36%   91.34%   -0.03%     
==========================================
  Files         164      163       -1     
  Lines       49733    49690      -43     
==========================================
- Hits        45439    45387      -52     
- Misses       4294     4303       +9
Flag Coverage Δ
#multiple 89.13% <ø> (-0.01%) ⬇️
#single 39.64% <ø> (-0.05%) ⬇️
Impacted Files Coverage Δ
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/core/frame.py 97.8% <0%> (-0.1%) ⬇️

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 c4a2cd3...151c40c. Read the comment docs.

@topper-123 topper-123 force-pushed the remove_pivot_nnual_isleapyear branch 3 times, most recently from 9252308 to 275c25a Compare November 21, 2017 00:15
@topper-123
Copy link
Contributor Author

I've moved test_daily and test_monthly to testPivotTable from TestPivotAnnual. test_hourly could be moved as well, but I'll do that tomorrow.

@jreback jreback added this to the 0.22.0 milestone Nov 22, 2017
@jreback jreback merged commit 1543761 into pandas-dev:master Nov 22, 2017
@jreback
Copy link
Contributor

jreback commented Nov 22, 2017

thanks @topper-123

@jreback
Copy link
Contributor

jreback commented Nov 22, 2017

@topper-123 ok for a followup to move tests if you want.

@topper-123 topper-123 deleted the remove_pivot_nnual_isleapyear branch November 27, 2017 23:59
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

Assignees
No one assigned
Labels
Deprecate Functionality to remove in pandas
Projects
None yet
Milestone
0.23.0
Development

Successfully merging this pull request may close these issues.
3 participants
@topper-123 @pep8speaks @jreback