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

DEPR: Rolling.win_type returning freq & is_datetimelike #38963

Merged
merged 9 commits into from
Jan 6, 2021

Conversation

mroeschke
Copy link
Member

@mroeschke mroeschke commented Jan 5, 2021

  • tests added / passed
  • Ensure all linting tests pass, see here for how to run them
  • whatsnew entry

xref: #38641 (comment)
xref: https://github.com/pandas-dev/pandas/pull/38664/files#r551419130

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 looking into this!

Question: is there a reason that it can't keep returning "freq" for the win_type ? (You don't seem to have to change anything else by making self.win_type return "freq" again.)
Or is it just that we don't need it internally?

def test_win_type_freq_return_deprecation():
freq_roll = Series(range(2), index=date_range("2020", periods=2)).rolling("2s")
with tm.assert_produces_warning(FutureWarning):
freq_roll.win_type
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
freq_roll.win_type
assert freq_roll.win_type == "freq"

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Done

@jreback jreback added Deprecate Functionality to remove in pandas Window rolling, ewma, expanding labels Jan 5, 2021
@jreback jreback modified the milestones: 1.2.1, 1.3 Jan 5, 2021
@mroeschke mroeschke changed the title DEPR: Rolling.win_type returning freq DEPR: Rolling.win_type returning freq & is_datetimelike Jan 5, 2021
@mroeschke
Copy link
Member Author

Yes win_type="freq" was an internally managed flag that we don't need anymore.

Also since we document in https://pandas.pydata.org/pandas-docs/stable/reference/api/pandas.DataFrame.rolling.html

If win_type=None, all points are evenly weighted; otherwise, win_type can accept a string of any scipy.signal window function.

I think its better for consistency that self.win_type is only a valid scipy.signal window function

@jorisvandenbossche
Copy link
Member

OK, that makes sense.

Another change that I noticed is the value stored in Rolling.window:

In [5]: freq_roll = Series(range(2), index=date_range("2020", periods=2)).rolling("2s")

In [6]: pd.__version__
Out[6]: '1.2.0'

In [7]: freq_roll.win_type
Out[7]: 'freq'

In [8]: freq_roll.window
Out[8]: 2000000000

In [9]: freq_roll.win_freq
Out[9]: '2s'

vs master:

In [1]: freq_roll = Series(range(2), index=date_range("2020", periods=2)).rolling("2s")

In [2]: freq_roll.win_type

In [3]: freq_roll.window
Out[3]: '2s'

So Rolling.window changed from an int (nanoseconds I suppose?) to the freq string. A change that makes sense I think (because the nanoseconds are not really useful, you need it as a Timedelta / actual window or so anyway).

But, that also makes it hard to follow the suggestion of "Check the type of self.window instead." in the deprecation warning (since this changed as well, or at least that means you need an actual pandas version check to do something different based on the pandas version, so not insurmountable in the end ;))

@jorisvandenbossche
Copy link
Member

The doc build is failing, we seem to be checking the win_type when its "freq", so might need to catch the warning there (didn't check how it is triggered):

>>>-------------------------------------------------------------------------
Warning in /home/runner/work/pandas/pandas/doc/source/user_guide/window.rst at block ending on line 215
Specify :okwarning: as an option in the ipython:: block to suppress this message
----------------------------------------------------------------------------
/home/runner/work/pandas/pandas/pandas/core/groupby/base.py:30: FutureWarning: win_type will no longer return 'freq' in a future version. Check the type of self.window instead.
  kwargs[attr] = getattr(self, attr)
<<<-------------------------------------------------------------------------

@mroeschke
Copy link
Member Author

Yeah the warning is occurring when a _shallow_copy is called with a time-based window because win_type is an attribute that needs be copied.

Should I just be suppressing that warning in _shallow_copy?

@jorisvandenbossche
Copy link
Member

Yeah, either catching/suppressing the warning, or either explicitly checking for the "win_type" attribute and in that case check the window and not include win_type if window is a time-based string? (if that's possible) Either is fine for me.

BTW, tested this branch with dask, and the tests are passing again (with warnings). And should be quite straightforward to update it to work on pandas master without warning as well.

Comment on lines 137 to 138
warnings.simplefilter("ignore", FutureWarning)
super()._shallow_copy(obj, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
warnings.simplefilter("ignore", FutureWarning)
super()._shallow_copy(obj, **kwargs)
with warnings.catch_warnings():
warnings.filterwarnings("ignore", "win_type", FutureWarning)
super()._shallow_copy(obj, **kwargs)

this should ensure the warning filter is only changed inside that context
this should ensure

@jreback jreback merged commit 7f5b669 into pandas-dev:master Jan 6, 2021
@jreback
Copy link
Contributor

jreback commented Jan 6, 2021

thanks @mroeschke

@mroeschke mroeschke deleted the depr/win_type branch January 6, 2021 18:25
@jorisvandenbossche
Copy link
Member

Thanks @mroeschke !

@@ -161,6 +161,8 @@ Deprecations
- Deprecated :meth:`MultiIndex.is_lexsorted` and :meth:`MultiIndex.lexsort_depth` as a public methods, users should use :meth:`MultiIndex.is_monotonic_increasing` instead (:issue:`32259`)
- Deprecated keyword ``try_cast`` in :meth:`Series.where`, :meth:`Series.mask`, :meth:`DataFrame.where`, :meth:`DataFrame.mask`; cast results manually if desired (:issue:`38836`)
- Deprecated comparison of :class:`Timestamp` object with ``datetime.date`` objects. Instead of e.g. ``ts <= mydate`` use ``ts <= pd.Timestamp(mydate)`` or ``ts.date() <= mydate`` (:issue:`36131`)
- Deprecated :attr:`Rolling.win_type` returning ``"freq"`` (:issue:`38963`)
Copy link
Member

Choose a reason for hiding this comment

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

small future note: since pd.Rolling does not exist, such a :attr:`Rolling.win_type` does not work

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah okay thanks for the note.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deprecate Functionality to remove in pandas Window rolling, ewma, expanding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants