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

DEPR/CLN: Remove freq parameters from df.rolling/expanding/ewm #18601

Merged
merged 1 commit into from Dec 6, 2017

Conversation

Projects
None yet
3 participants
@topper-123
Contributor

topper-123 commented Dec 2, 2017

-- [x ] tests added / passed

  • [x ] passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • [x ] whatsnew entry

The freq parameter of df.rolling/expanding/ewm was deprecated in 0.18 (#11603). This PR removes the parameter from the code base.

After this PR, I will remove the how parameter and lastly the pd.rolling_*, pd.expanding_* and pd.ewm_* will be removed (AKA pd.stats.*). By removing freq and howbefore pd.stats I think it will be easier to clean up pandas/tests/test_window.py, as ATM these three issues are not very cleanly separated in that test module.

In some test in test_window.py::TestMoments there is a bit of resampling going on, as I've moved freq stuff from rolling into a prior df.resample step. These are tests for how and will be removed once how is removed (unless the tests good for testing the windows functions, I'm not completely sure ATM, but will look into it when I reach that point).

Additionally (and unrelated), in pandas/tests/test_window.py there are checks for numpy>=1.8 and >=1.9. These checks are no longer necessary, as numpy 1.9 is the current minium version, so they're removed,.

@@ -133,6 +133,8 @@ Removal of prior version deprecations/changes
- ``pd.tseries.util.pivot_annual`` has been removed (deprecated since v0.19). Use ``pivot_table`` instead (:issue:`18370`)
- ``pd.tseries.util.isleapyear`` has been removed (deprecated since v0.19). Use ``.is_leap_year`` property in Datetime-likes instead (:issue:`18370`)
- ``pd.ordered_merge`` has been removed (deprecated since v0.19). Use ``pd.merge_ordered`` instead (:issue:`18459`)
- The ``freq`` parameter has been removed from the ``rolling``/``expanding``/``ewm`` methods of DataFrame
and Series. Instead, resample before calling the methods. (:issue:xxx)

This comment has been minimized.

@gfyoung

gfyoung Dec 3, 2017

Member

Reference the original PR where it was deprecated.

@gfyoung

This comment has been minimized.

Member

gfyoung commented Dec 3, 2017

@topper-123 : Thanks for this! Just need to patch a flake8 issue, and you should be good.

@jreback jreback referenced this pull request Dec 3, 2017

Open

DEPR: deprecations log for removed issues #13777

115 of 115 tasks complete
@codecov

This comment has been minimized.

codecov bot commented Dec 3, 2017

Codecov Report

Merging #18601 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18601      +/-   ##
==========================================
- Coverage   91.46%   91.44%   -0.03%     
==========================================
  Files         157      157              
  Lines       51439    51426      -13     
==========================================
- Hits        47051    47028      -23     
- Misses       4388     4398      +10
Flag Coverage Δ
#multiple 89.31% <100%> (-0.01%) ⬇️
#single 40.61% <77.77%> (-0.1%) ⬇️
Impacted Files Coverage Δ
pandas/core/window.py 96.31% <100%> (-0.07%) ⬇️
pandas/stats/moments.py 70.81% <100%> (-0.39%) ⬇️
pandas/core/generic.py 95.9% <100%> (ø) ⬆️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/core/frame.py 97.81% <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 0e16818...010cf95. Read the comment docs.

@codecov

This comment has been minimized.

codecov bot commented Dec 3, 2017

Codecov Report

Merging #18601 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18601      +/-   ##
==========================================
+ Coverage   91.58%   91.58%   +<.01%     
==========================================
  Files         153      153              
  Lines       51250    51237      -13     
==========================================
- Hits        46935    46924      -11     
+ Misses       4315     4313       -2
Flag Coverage Δ
#multiple 89.44% <100%> (+0.01%) ⬆️
#single 40.68% <45%> (-0.1%) ⬇️
Impacted Files Coverage Δ
pandas/core/window.py 96.31% <100%> (-0.07%) ⬇️
pandas/core/generic.py 95.9% <100%> (ø) ⬆️
pandas/stats/moments.py 70.81% <100%> (-0.39%) ⬇️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/core/frame.py 97.81% <0%> (-0.1%) ⬇️
pandas/plotting/_converter.py 65.25% <0%> (+1.81%) ⬆️

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 c3c04e2...c7ae78e. Read the comment docs.

@topper-123

This comment has been minimized.

Contributor

topper-123 commented Dec 3, 2017

all green. The travis error seems unrelated to this PR.

@jreback

couple of comments

frequency by resampling the data. This is done with the default parameters
of :meth:`~pandas.Series.resample` (i.e. using the `mean`).
See Also
---------

This comment has been minimized.

@jreback

jreback Dec 3, 2017

Contributor

I think these need to be the same length as the text (the underlines)

This comment has been minimized.

@topper-123

topper-123 Dec 3, 2017

Contributor

Actually, it has to be the same length or longer. But yeah, it's probably better style to keep same length. I'll change it.

@@ -208,6 +208,7 @@ def ensure_compat(dispatch, name, arg, func_kw=None, *args, **kwargs):
if value is not None:
kwds[k] = value
kwargs.pop('freq', None) # freq removed in 0.22

This comment has been minimized.

@jreback

jreback Dec 3, 2017

Contributor

this shouldn't be necessary

This comment has been minimized.

@topper-123

topper-123 Dec 3, 2017

Contributor

A bunch of tests in pandas\tests\test_window.py fail without this.

As I'll clean up that module after finishing up how (incl. all reference to freq for pd.stats.*), I suggest leaving it as it is. It not worth it to make the tests pass without this for a module that will be deleted shortly.

This comment has been minimized.

@jreback

jreback Dec 3, 2017

Contributor

that's fine, just add a TODO then

This comment has been minimized.

@topper-123

topper-123 Dec 3, 2017

Contributor

Well, the whole moments.py module will be deleted, incl. this small change....

But ok, I'ved added it.

This comment has been minimized.

@jreback

jreback Dec 3, 2017

Contributor

I understand, and until that time, pls add a TODO

@topper-123

This comment has been minimized.

Contributor

topper-123 commented Dec 4, 2017

All green.

@jreback

This comment has been minimized.

Contributor

jreback commented Dec 4, 2017

lint error & needs rebase

@topper-123

This comment has been minimized.

Contributor

topper-123 commented Dec 4, 2017

green, I think.

Wrt. linting, I don't get any warning locally. There is a warning that pops up, but that is already in master, so unrelated to my PR (unless I've misunderstood something).

Next I will submit a PR on how, that is already mostly done.

@jreback

This comment has been minimized.

Contributor

jreback commented Dec 4, 2017

linting is an issue on this PR
see the travis log

you can show it locally with
make lint-diff

@topper-123

This comment has been minimized.

Contributor

topper-123 commented Dec 5, 2017

Thanks, I've seen it on travis now.

Can't get make lint-diff to work (tried windows command prompt and MINGW64). is it a linux command?

@jreback

This comment has been minimized.

Contributor

jreback commented Dec 5, 2017

yeah sorry it’s a linux cmd
the same command is ithe contributing guide and has a windows variant

@topper-123

This comment has been minimized.

Contributor

topper-123 commented Dec 5, 2017

All green.

@topper-123 topper-123 changed the title from DEPR/CLN: Remove freq keyword from df.rolling/expanding/ewm to DEPR/CLN: Remove freq and how parameters from df.rolling/expanding/ewm Dec 5, 2017

@topper-123 topper-123 changed the title from DEPR/CLN: Remove freq and how parameters from df.rolling/expanding/ewm to DEPR/CLN: Remove freq parameters from df.rolling/expanding/ewm Dec 5, 2017

@jreback jreback added this to the 0.22.0 milestone Dec 6, 2017

@jreback

jreback approved these changes Dec 6, 2017

@jreback jreback merged commit 5665a3e into pandas-dev:master Dec 6, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jreback

This comment has been minimized.

Contributor

jreback commented Dec 6, 2017

thanks @topper-123

@topper-123 topper-123 deleted the topper-123:remove_rolling_etc_freq branch Dec 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment