BUG: Fix freq setter for DatetimeIndex/TimedeltaIndex and deprecate for PeriodIndex
State: closed

- [X] closes #20678
- [X] tests added / passed
- [X] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [X] whatsnew entry

For `DatetimeIndex`/`TimedeltaIndex`:
  - Setting `freq` with a string alias now works
  - Frequencies are validated with invalid frequencies raising
     - Same behavior as when the constructor is called with an invalid frequency

For `PeriodIndex`:
  - Setting `freq` is deprecated in favor of `asfreq`
      - The behavior of the setter would be ambiguous without the extra `asfreq` params
  - Left the existing (buggy) `freq` setter behavior as-is
     - Didn't seem worthwhile to try fixing it
  - I don't think that the setter was widely used for `PeriodIndex` anyways
     - Didn't see any warnings in the tests

Labels:
name: Bug
name: Frequency
description: DateOffsets
name: Deprecate
description: Functionality to remove in pandas

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/20772?src=pr&el=h1) Report
> Merging [#20772](https://codecov.io/gh/pandas-dev/pandas/pull/20772?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/563a6ad108a79b0192a25d51d743812a18e21b15?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/20772/graphs/tree.svg?token=eZ4WkYLtcO&src=pr&width=650&height=150)](https://codecov.io/gh/pandas-dev/pandas/pull/20772?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #20772      +/-   ##
==========================================
+ Coverage   91.77%   91.78%   +<.01%     
==========================================
  Files         153      153              
  Lines       49313    49322       +9     
==========================================
+ Hits        45259    45268       +9     
  Misses       4054     4054
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.17% <100%> (ø)` | :arrow_up: |
| #single | `41.9% <83.33%> (+0.01%)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/20772?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/indexes/period.py](https://codecov.io/gh/pandas-dev/pandas/pull/20772/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9wZXJpb2QucHk=) | `92.67% <100%> (+0.06%)` | :arrow_up: |
| [pandas/core/indexes/datetimelike.py](https://codecov.io/gh/pandas-dev/pandas/pull/20772/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZWxpa2UucHk=) | `96.79% <100%> (+0.09%)` | :arrow_up: |
| [pandas/core/indexes/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/20772/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZXMucHk=) | `95.73% <100%> (-0.04%)` | :arrow_down: |
| [pandas/core/indexes/timedeltas.py](https://codecov.io/gh/pandas-dev/pandas/pull/20772/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy90aW1lZGVsdGFzLnB5) | `91.15% <100%> (-0.07%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/20772?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/20772?src=pr&el=footer). Last update [563a6ad...68b8b99](https://codecov.io/gh/pandas-dev/pandas/pull/20772?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

I would also deprecate settig for DTI/TDI, and direct to using ``.asfreq``. I don't like the idea of disallowing on PI but not the others.
Can implement `.asfreq` and deprecate the setters for DTI/TDI in the next day or so.

cc @jorisvandenbossche : Thoughts on this?  Your preference in the issue was to allow setting for DTI/TDI and not implement `.asfreq`.
Yes, -1 on `asfreq`. As I tried to argue on the issue, it is something different as `PeriodIndex.asfreq`, so we should not call it the same. Directly setting the attribute is fine IMO if you need this.
> I don't like the idea of disallowing on PI but not the others.

I think this difference is acceptable, because `freq` also has a different notion for both: for PI the `freq` is required (you cannot have a PI without a `freq`), and the underlying int values are dependent on this freq. So changing the `freq` is either buggy because the values and the freq no longer match (current behaviour), or it converts the underlying int values to match the new `freq` (but this is what `asfreq` already does, and since this alters the data, I think should not be done by setting an attribute). 
On the other hand, for DTI and TDI, the `freq` is not required, and setting it allows you to add one. But this only changes the `freq` to the `inferred_freq` and does not alter the data (as otherwise an error is raised).
(note: I tagged for 0.23, but it does not need to hold up the rc)
> (note: I tagged for 0.23, but it does not need to hold up the rc)

If this is going in 0.23, then I'd like it to be in the RC. Ideally we only merge bugfixes for the RC to the 0.23 release branch.
I more or less have code ready that addresses @jreback's comments, but am a bit hesitant to push since there isn't agreement on if the freq setter should be deprecated, and if so what the name of the alternative method would be.
This commit would go on top of the current changes in this PR to address @jreback's comments, in case anyone cares too look while things are still being discussed here: https://github.com/jschendel/pandas/commit/2fca7751205267bcc3fa1c07de9af19d646b5f6a

The CI services that are hooked to my repo all ran successfully, and I didn't see any warnings related to my changes in the logs.  Didn't want to mess around with this PR too much while there's still ongoing discussion.
So I have a clear -1 on re-using the name `asfreq` for this, as I explained before (asfreq on Period is doing something different, I haven't see anybody giving answers to that argument).

If we want to keep the possibility of setting the freq, I don't mind it being the direct setting of the freq attribute (I don't think it is worth to introduce another method for this)

> the inconcistenty with that you cannot set PI is pretty jaring.

I don't mind allowing setting on a PI. But if we follow the same rules as for setting `freq` on DTI/TDI (= only a freq that matches the data, so it does not change the interpretation of the data), then for PI this would mean that only the `freq` it already has will work, all other freqs will raise an exception. 
I am fine with allowing that if you really want that for consistency, but it's not particularly useful.

> So I have a clear -1 on re-using the name asfreq for this, as I explained before (asfreq on Period is doing something different, I haven't see anybody giving answers to that argument).

Can you repeat I can't find what you said.

I am fine the the PI change here. The issue comes down to whether we should allow one to change ``freq`` on the *immutable* DTI/TDI. I see no good reason for this. If you want to set the freq, simply create another one, or we make availble ``.asfreq`` on TDI/DTI (which I am little less excited about). I think having a method only on PI is fine. However the *setting* of an attribute is just plane wrong (yes we allow it for ``.name``, but recently removed it for ``.tz`` as that was buggy).


I have said it here https://github.com/pandas-dev/pandas/pull/20772#issuecomment-385060944 and here https://github.com/pandas-dev/pandas/issues/20678#issuecomment-381050890. But it actually just boils down to "for PI `asfreq` *changes* the underlying data (it is for conversion of one frequency to another), for DTI/TDI setting the `freq` just updates an informative attribute that does not change the data at all"

>  However the *setting* of an attribute is just plane wrong 

There is nothing "plain wrong" about setting an attribute, it is perfectly valid and depending on the use case often even preferable python code (I think often having a lot of "set_.." methods is seen as "old-java-style" while just having a property with getter and setter under the hood is more pythonic).
> However the setting of an attribute is just plane wrong

On an immutable object this is True. We have 1 and only 1 attribute that we allow to be changed on an Index and that is the name. All others give you back a new object. Why break this paradigm?

Further I don't buy your argument. Whether ``.asfreq`` happens to do work on a PI, and doesn't on a TDI/DTI is not a compelling argument.

Is there a compelling use-case to actually set freq (whether inplace or return a new object) on a DTI/TDI?
> Is there a compelling use-case to actually set freq (whether inplace or return a new object) on a DTI/TDI?

I made this PR after noticing a few SO questions that worked around the buggy setting behavior.  That doesn't necessarily there's actually a _valid_ use case for it, as there may be more direct workarounds for the issues people are having, but at the very least people are trying to use it.

I think one of the issues is that people generally default to using `pd.to_datetime` for parsing, which as far as I can tell does not have the ability to set a frequency on creation, and so they then want to set a frequency after creation.  This might be more of an argument for a `freq` parameter in `pd.to_datetime` though.  There also isn't much documentation for the `freq='infer'` option of the DTI constructor, so I also see people wanting to set the inferred frequency after creation, without actually knowing what that frequency is (again, more of a documentation issue).

I could also imagine a use case where one has messy data which needs to be cleaned first, prior to actually setting the `freq`.  So a workflow like parse -> clean -> set frequency.  Some cleaning operations may take care of the setting the `freq`, but there are cases where multiple frequencies are possible, so the correct one may not be set, or a custom frequency may be desired, or maybe no frequency is desired and a user wants to remove by setting `None`.


>Whether .asfreq happens to do work on a PI, and doesn't on a TDI/DTI is not a compelling argument.

To add another data point, `Series.asfreq` and `DataFrame.asfreq` will also do work on an underlying DTI (doesn't appear to work for TDI).

Not particularly invested in being for/against reusing the `asfreq` name though, as there are downsides to both without a clear winner in my mind (potential for confusion with reuse vs. API bloat for new method).
@jschendel thanks for the color. But *why* are people actually setting the ``freq``. This doesn't actually do anything (I don't think). Anytime we need the freq we actually infer it (again for DTI/TDI)
Setting the `freq` enables a couple operations that fail without one:

```python
In [2]: dti = pd.DatetimeIndex(['20180101', '20180102', '20180103'])

In [3]: dti
Out[3]: DatetimeIndex(['2018-01-01', '2018-01-02', '2018-01-03'], dtype='datetime64[ns]', freq=None)

In [4]: dti.shift(1)
---------------------------------------------------------------------------
NullFrequencyError: Cannot shift with no freq

In [5]: dti + 4
---------------------------------------------------------------------------
NullFrequencyError: Cannot shift with no freq

In [6]: dti = pd.DatetimeIndex(['20180101', '20180102', '20180103'], freq='B')

In [7]: dti.shift(1)
Out[7]: DatetimeIndex(['2018-01-02', '2018-01-03', '2018-01-04'], dtype='datetime64[ns]', freq='B')

In [8]: dti + 4
Out[8]: DatetimeIndex(['2018-01-05', '2018-01-08', '2018-01-09'], dtype='datetime64[ns]', freq='B')
```

I've seen people use operations like this to enable some shorthand for inserting the previous/next logical value into an index, e.g. `dti.insert(0, dti[0] - 1)`
right, we allow an integer to stand in an represent the freq. ok I would say these are ok usecases (if prob not very well documented). but that is not sufficient reason to actually allow setting of an attribute on an immutable object (rather than just having a function give you a new one). its just bad semantics.

I would be ok with

``dti.asfreq('D')``, makes it pretty explict. We have methods for setting things like tz's, why not freq.
> On an immutable object this is True. We have 1 and only 1 attribute that we allow to be changed on an Index and that is the name. All others give you back a new object. Why break this paradigm?

Exactly because the `freq` does *not change* the index, so the index is still immutable, except for those informative attributes like `name` and `freq`. So that in sense, `freq` is very similar to `name` for a DTI/TDI.

> To add another data point, Series.asfreq and DataFrame.asfreq will also do work on an underlying DTI (doesn't appear to work for TDI).

For me that is another reason not to use `asfreq`, as for Series this is indeed a basic form of resample. So the name `asfreq` is already too much overloaded, no need to add yet another meaning.
ok, so I would be on board with deprecating ``.freq`` setting as in this PR, and adding a ``.set_freq()`` as an alternative (DTI/TDI, I don't think this makes sense for PI).

 This would be much more in line with our handling of names in any event. The PI changes are fine.

Agree that ``.asfreq`` might be confusing here for DTI/TDI.
@jreback Are you fine with merging this now as is? The changes it does are already good (fixing a clear bug in the current setting functionality + deprecation for PI), and the other changes you propose in your last comment can be done separately I think  (on top of this PR). 

(just as way to include this in the rc)
yes ok to merge now (and let’s open an issue though)
Opened https://github.com/pandas-dev/pandas/issues/20886 for the rest.
Thanks all.