DEPR: Timestamp.freq #41586

jbrockmendel · 2021-05-20T17:15:39Z

closes DEPR: remove freq attribute of Timestamp? #15146
tests added / passed
Ensure all linting tests pass, see here for how to run them
whatsnew entry

This in turn will allow us to remove DatetimeArray.freq and TimedeltaArray.freq, which is needed for #31218

phofl · 2021-05-20T21:11:38Z

Changed your post to avoid closing the other issue too

jreback · 2021-05-21T15:47:26Z

doc/source/reference/offset_frequency.rst

@@ -1309,6 +1515,12 @@ Methods
    Nano.__call__
    Nano.apply
    Nano.apply_index
+    Nano.is_month_start


hmm this is going to generate a huge amount of pages, IIRC we do this just on the base class and that becomes generic. cc @jorisvandenbossche @datapythonista

Yeah, I would prefer not adding those duplicated for each of the offsets (that actually also applies to the existing duplicated methods/attributes). We should follow the pattern used eg by the Index classes where only the subclass-specific ones are listed for non-base Index classes.

opened #41745 to try to trim down the existing properties

jreback · 2021-05-21T15:48:55Z

pandas/_libs/tslibs/timestamps.pyx

@@ -1647,12 +1703,18 @@ default 'raise'
            value = tz_localize_to_utc_single(self.value, tz,
                                              ambiguous=ambiguous,
                                              nonexistent=nonexistent)
-            return Timestamp(value, tz=tz, freq=self.freq)
+            out = Timestamp(value, tz=tz)


can we some just avoid setting this?

jreback · 2021-05-21T15:49:43Z

pandas/core/arrays/datetimes.py

+            # filter out warnings about Timestamp.freq
+            warnings.filterwarnings("ignore", category=FutureWarning)
+
+            return Timestamp(x, freq=self.freq, tz=self.tz)


can we not just set this to None? (or we have to because you are deprecating)

if we could change this without a deprecation cycle itd be convenient, but that would change what you get from dti[0]

Can you use the internal _set_freq after construction instead of the catch_warnings?
Iteratively calling catch_warnings many times will give a lot of overhead (although we don't use this ourselves in something like __iter__ where we need to box many objects, but the user could still do something similar themselves with getitem?)

jorisvandenbossche

Accessing the .freq attribute of a Timestamp instance itself is not yet deprecated? That should also be deprecated to be able to remove it?

jorisvandenbossche · 2021-05-26T13:12:53Z

doc/source/reference/offset_frequency.rst

@@ -1309,6 +1515,12 @@ Methods
    Nano.__call__
    Nano.apply
    Nano.apply_index
+    Nano.is_month_start


Yeah, I would prefer not adding those duplicated for each of the offsets (that actually also applies to the existing duplicated methods/attributes). We should follow the pattern used eg by the Index classes where only the subclass-specific ones are listed for non-base Index classes.

jorisvandenbossche · 2021-05-26T13:16:41Z

doc/source/whatsnew/v1.3.0.rst

@@ -682,6 +682,7 @@ Deprecations
 - Deprecated passing arguments as positional in :meth:`DataFrame.clip` and :meth:`Series.clip` (other than ``"upper"`` and ``"lower"``) (:issue:`41485`)
 - Deprecated special treatment of lists with first element a Categorical in the :class:`DataFrame` constructor; pass as ``pd.DataFrame({col: categorical, ...})`` instead (:issue:`38845`)
 - Deprecated passing arguments as positional (except for ``"method"``) in :meth:`DataFrame.interpolate` and :meth:`Series.interpolate` (:issue:`41485`)
+- Deprecated the :attr:`Timestamp.freq` attribute.  For the properties that use it (``is_month_start``, ``is_month_end``, ``is_quarter_start``, ``is_quarter_end``, ``is_year_start``, ``is_year_end``), when you have a ``freq``, use e.g. ``freq.is_month_start(ts)`` (:issue:`15146`)


I don't find the explanation of "for properties that use it .., when you have a freq, ..." very clear. Adding an actual example might clarify.

First, I assume people who do ts.is_month_start (with ts being a Timestamp) don't necessarily know that they are relying on some freq.
Also, the attributes are not going away, right? It's only the fastpath for computing it based on the freq that goes away? Are there actually cases that will change behaviour, or is it only performance? (and if only performance, that might not matter much for scalars, or maybe not worth warning about).

Are there actually cases that will change behaviour, or is it only performance?

Yes, e.g.

dti = pd.date_range("2017-01-01", periods=30, freq="B") dti[0].is_month_start # <- True now, False once freq is removed

And in principle we can know when this will change? (depending on the freq, we can know whether it can give different answers?)
Could we warn only in those cases?

Good idea, will do

Updated to warn in fewer cases. Still haven't updated the whatsnew note per the first comment here. suggestions on wording?

i agree here. i think we are simply deprecating freq. why do we need to even say anything about is_month_start for example?

bc the result of is_month_start is going to change in some cases

One consequence of deprecating Timestamp.freq is that when having a timeseries with a freq, the following equivalency won't no longer always hold:

dtidx.is_month_start[0] == dtidx[0].is_month_start

That's a bit unfortunate, I would say, and probably something we should explicitly document.

ok for this as a followup documentation (but for 1.3)

jorisvandenbossche · 2021-05-26T13:17:44Z

pandas/_libs/tslibs/timestamps.pyx

+            "Timestamp.freq is deprecated and will be removed in a future version. "
+            "When you have a freq, use freq.is_month_start(timestamp) instead",
+            FutureWarning,
+            stacklevel=2,


Same as above, I think this warning message is rather vague / unclear (but let's discuss above)

pandas/_libs/tslibs/timestamps.pyx

jorisvandenbossche · 2021-05-26T13:26:35Z

pandas/core/arrays/datetimes.py

+            # filter out warnings about Timestamp.freq
+            warnings.filterwarnings("ignore", category=FutureWarning)
+
+            return Timestamp(x, freq=self.freq, tz=self.tz)


Can you use the internal _set_freq after construction instead of the catch_warnings?
Iteratively calling catch_warnings many times will give a lot of overhead (although we don't use this ourselves in something like __iter__ where we need to box many objects, but the user could still do something similar themselves with getitem?)

jreback · 2021-05-31T15:37:31Z

this would be good to get in the rc

jbrockmendel · 2021-05-31T15:55:14Z

this would be good to get in the rc

what's the soft-deadline for this?

jreback · 2021-05-31T16:03:26Z

this would be good to get in the rc

what's the soft-deadline for this?

soonish

cc @simonjayhawkins

…freq

jorisvandenbossche · 2021-06-01T21:05:42Z

Repeating my non-inline review comment again here to ensure it was noticed:

Accessing the .freq attribute of a Timestamp instance itself is not yet deprecated? That should also be deprecated to be able to remove it?

jbrockmendel · 2021-06-01T22:42:22Z

Repeating my non-inline review comment again here to ensure it was noticed:

Thanks, will get to that shortly.

simonjayhawkins · 2021-06-02T10:06:19Z

this would be good to get in the rc

what's the soft-deadline for this?

soonish

I'll put the blocker on this also for now.

will release 1.2.5 first. so still got a few days for this.

pandas/_libs/tslibs/timestamps.pyx

jorisvandenbossche · 2021-06-04T12:12:17Z

pandas/tests/frame/methods/test_reset_index.py

@@ -50,6 +50,7 @@ def test_set_index_reset_index_dt64tz(self):
        df = result.set_index("foo")
        tm.assert_index_equal(df.index, idx)

+    @pytest.mark.filterwarnings("ignore:Timestamp.freq is deprecated:FutureWarning")


What's causing this test to raise a warning?

looks like it doesnt after one of the more recent comments, good catch, will remove

jorisvandenbossche · 2021-06-04T12:15:27Z

pandas/tests/scalar/timestamp/test_arithmetic.py

@@ -16,6 +16,10 @@

 import pandas._testing as tm

+pytestmark = pytest.mark.filterwarnings(
+    "ignore:Timestamp.freq is deprecated:FutureWarning"


Can you apply this mark to only the tests that need it? (I would expect not all tests to need it?)

good idea, will do

jreback · 2021-06-04T20:30:13Z

doc/source/whatsnew/v1.3.0.rst

@@ -682,6 +682,7 @@ Deprecations
 - Deprecated passing arguments as positional in :meth:`DataFrame.clip` and :meth:`Series.clip` (other than ``"upper"`` and ``"lower"``) (:issue:`41485`)
 - Deprecated special treatment of lists with first element a Categorical in the :class:`DataFrame` constructor; pass as ``pd.DataFrame({col: categorical, ...})`` instead (:issue:`38845`)
 - Deprecated passing arguments as positional (except for ``"method"``) in :meth:`DataFrame.interpolate` and :meth:`Series.interpolate` (:issue:`41485`)
+- Deprecated the :attr:`Timestamp.freq` attribute.  For the properties that use it (``is_month_start``, ``is_month_end``, ``is_quarter_start``, ``is_quarter_end``, ``is_year_start``, ``is_year_end``), when you have a ``freq``, use e.g. ``freq.is_month_start(ts)`` (:issue:`15146`)


ok for this as a followup documentation (but for 1.3)

jreback · 2021-06-04T20:31:39Z

pandas/core/arrays/datetimes.py

+                    start_i = i * chunksize
+                    end_i = min((i + 1) * chunksize, length)
+                    converted = ints_to_pydatetime(
+                        data[start_i:end_i], tz=self.tz, freq=self.freq, box="timestamp"


can you just NOT pass self.freq?

Not without changing the behavior. ATM next(iter(dti)).freq matches dti.freq. Not passing freq here would change that

jreback · 2021-06-04T20:32:25Z

pandas/core/arrays/datetimelike.py

@@ -1189,7 +1189,11 @@ def _addsub_object_array(self, other: np.ndarray, op):
        # Caller is responsible for broadcasting if necessary
        assert self.shape == other.shape, (self.shape, other.shape)

-        res_values = op(self.astype("O"), np.asarray(other))
+        with warnings.catch_warnings():


shouln't these not have freq to begin with?

self here is DTA/TDA/PA, which all have freq. there is an issue about removing freq from DTA/TDA, which this deprecation is a blocker for

jreback · 2021-06-07T16:47:57Z

merging, @jbrockmendel can you followup with respect to any open comments (i think just the whatsnew but maybe more)

jorisvandenbossche · 2021-06-07T18:41:26Z

pandas/tests/frame/methods/test_reset_index.py

@@ -421,6 +421,7 @@ def test_reset_index_multiindex_columns(self):
        result = df2.rename_axis([("c", "ii")]).reset_index(col_level=1, col_fill="C")
        tm.assert_frame_equal(result, expected)

+    @pytest.mark.filterwarnings("ignore:Timestamp.freq is deprecated:FutureWarning")
    def test_reset_index_datetime(self, tz_naive_fixture):


Similar question as before: what's causing this test to raise a warning?

jorisvandenbossche · 2021-06-07T18:44:49Z

pandas/tests/reshape/concat/test_datetimes.py

@@ -403,6 +403,7 @@ def test_concat_multiple_tzs(self):
        expected = DataFrame({"time": [ts2, ts3]})
        tm.assert_frame_equal(results, expected)

+    @pytest.mark.filterwarnings("ignore:Timestamp.freq is deprecated:FutureWarning")
    def test_concat_multiindex_with_tz(self):


Same here, what's causing this test to raise a warning? (I don't directly see a .freq access)

jorisvandenbossche · 2021-06-07T18:45:11Z

pandas/tests/reshape/test_pivot.py

@@ -516,6 +516,7 @@ def test_pivot_index_with_nan(self, method):
            result = pd.pivot(df, "b", "a", "c")
        tm.assert_frame_equal(result, pv.T)

+    @pytest.mark.filterwarnings("ignore:Timestamp.freq is deprecated:FutureWarning")


looks like in all three cases they no longer are needed. will remove in my next follow-up branch

simonjayhawkins · 2021-07-01T16:21:20Z

This resulted in a performance regression https://pandas.pydata.org/speed/pandas/#tslibs.timestamp.TimestampProperties.time_freqstr?Cython=0.29.21&jinja2=&python=3.8

however, we will be ignoring this #40008 (comment)

BTW looking at the regressions listed, I think we can hit "ignore" for the TimestampProperties.time_freqstr ones, since those are driven by a deprecation of freqstr (and are still measured in ns)

jbrockmendel added 2 commits May 20, 2021 10:13

DEPR: Timestamp.freq

07a131d

whatsnew

7f231e7

jbrockmendel added Deprecate Functionality to remove in pandas Frequency DateOffsets labels May 20, 2021

update docs

a511ceb

jbrockmendel added 2 commits May 20, 2021 15:49

typo fixup

8f4666d

Merge branch 'master' into depr-freq

913ed4b

jreback requested changes May 21, 2021

View reviewed changes

Merge branch 'master' into depr-freq

d222341

simonjayhawkins added this to the 1.3 milestone May 24, 2021

jbrockmendel and others added 3 commits May 25, 2021 21:29

Merge branch 'master' into depr-freq

1cf43a8

Merge branch 'master' into depr-freq

2df27d2

Merge branch 'master' into depr-freq

e5edf28

jorisvandenbossche requested changes May 26, 2021

View reviewed changes

jbrockmendel mentioned this pull request May 31, 2021

DOC: collect positional deprecations, remove duplicate DateOffset methods #41745

Closed

jbrockmendel added 4 commits May 31, 2021 09:25

Merge branch 'master' into depr-freq

1b8bc03

Warn in fewer cases

2f8e61b

Merge branch 'master' into depr-freq

7d9d524

Merge branch 'depr-freq' of github.com:jbrockmendel/pandas into depr-…

c0974c3

…freq

Merge branch 'master' into depr-freq

4066dd2

simonjayhawkins added the Blocker Blocking issue or pull request for an upcoming release label Jun 2, 2021

jbrockmendel added 2 commits June 2, 2021 10:58

Deprecated .freq, .freqstr lookups

3435954

Merge branch 'master' into depr-freq

b93b6c1

mypy fixup

069ae4c

jreback reviewed Jun 3, 2021

View reviewed changes

pandas/_libs/tslibs/timestamps.pyx Outdated Show resolved Hide resolved

de-duplicate warnings

e10a053

jorisvandenbossche reviewed Jun 4, 2021

View reviewed changes

jbrockmendel added 2 commits June 4, 2021 09:54

Merge branch 'master' into depr-freq

33df636

filterwarning more precisely

e6f1c6b

jreback requested changes Jun 4, 2021

View reviewed changes

simonjayhawkins mentioned this pull request Jun 7, 2021

RLS: 1.3 #40169

Closed

jreback approved these changes Jun 7, 2021

View reviewed changes

jreback merged commit dae5c59 into pandas-dev:master Jun 7, 2021

jbrockmendel deleted the depr-freq branch June 7, 2021 16:51

jorisvandenbossche reviewed Jun 7, 2021

View reviewed changes

jorisvandenbossche removed the Blocker Blocking issue or pull request for an upcoming release label Jun 15, 2021

JulianWgs pushed a commit to JulianWgs/pandas that referenced this pull request Jul 3, 2021

DEPR: Timestamp.freq (pandas-dev#41586)

b9397d3

jbrockmendel mentioned this pull request Jul 4, 2021

RLS: 1.3.1 #42343

Closed

DEPR: Timestamp.freq #41586

DEPR: Timestamp.freq #41586

Conversation

jbrockmendel commented May 20, 2021 • edited by phofl Loading

phofl commented May 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented May 31, 2021

jbrockmendel commented May 31, 2021

jreback commented May 31, 2021

jorisvandenbossche commented Jun 1, 2021

jbrockmendel commented Jun 1, 2021

simonjayhawkins commented Jun 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Jun 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonjayhawkins commented Jul 1, 2021

jbrockmendel commented May 20, 2021 •

edited by phofl

Loading