API: Implement set_freq for DTI/TDI, deprecate freq setter #20892
Conversation
| @@ -399,7 +400,6 @@ def test_daterange_bug_456(self): | |||
| # GH #456 | |||
| rng1 = bdate_range('12/5/2011', '12/5/2011') | |||
| rng2 = bdate_range('12/2/2011', '12/5/2011') | |||
| rng2.freq = BDay() | |||
There was a problem hiding this comment.
This is redundant, as bdate_range should already set this as the frequency. I couldn't find a test that verified this though, so I added a check to a test a few lines above. Same deal with the CDay() changes below.
jschendel
force-pushed
the
freq-setter-depr
branch
from
5370339
to
53db8fb
Compare
Codecov Report
@@ Coverage Diff @@
## master #20892 +/- ##
==========================================
+ Coverage 91.81% 91.81% +<.01%
==========================================
Files 153 153
Lines 49479 49491 +12
==========================================
+ Hits 45428 45441 +13
+ Misses 4051 4050 -1
Continue to review full report at Codecov.
|
| warnings.warn(msg, FutureWarning, stacklevel=2) | ||
| self.freq = value | ||
| if value is not None: | ||
| value = to_offset(value) |
There was a problem hiding this comment.
This is for the deprecated offset setter which is just an alias for the freq setter, so I don't think set_freq should be used here since setting is to happen inplace (as was the previous convention). This previously would call the freq setter, but since that's also being deprecated it was subsequently raising the freq setter warning too, so I just reused the freq setter code to avoid the additional warning.
|
Added |
jreback
added
API Design
Frequency
| if value is not None: | ||
| value = frequencies.to_offset(value) | ||
| self._validate_frequency(self, value) | ||
|
|
||
| self._freq = value | ||
|
|
||
| def set_freq(self, freq): |
There was a problem hiding this comment.
can you make this available on the .dt accessors as well
There was a problem hiding this comment.
This currently does nothing when used with the dt accessor due to #20937; the underlying frequency gets changed with set_freq, but upon Series construction the new frequency gets lost.
I've pushed a commit to add this to the dt accessor, and xfailed the corresponding tests I've added. Can revert the commit if this is no longer deemed important.
jschendel
force-pushed
the
freq-setter-depr
branch
2 times, most recently
from
8c6564b
to
1b7a5a9
Compare
jschendel
force-pushed
the
freq-setter-depr
branch
from
1b7a5a9
to
5778e28
Compare
|
nice PR @jschendel , lgtm. any comments @jorisvandenbossche @TomAugspurger |
|
@jorisvandenbossche you commented on the issue. but this PR looks pretty good to me, and it allows chained operations, something we like, not to mention |
|
This has a merge conflict in the release notes now. I'm 50/50 on the changes here, will wait for @jorisvandenbossche to review. |
|
As I commented on the issue, I still don't see the need or added value to add a new method for this (and yes, this is a nicely done PR, that's not my problem :-)). See #20886 (comment) BTW, I don't feel strongly about this (as it is something I will never use), so I am fine to go with the majority if there is one, I just have the feeling we are adding a new method for something we don't need (and the bar to add new methods should be higher) |
|
ok, closing this. thanks for the PR @jschendel we can revisit in the future if more interest. |
git diff upstream/master -u -- "*.py" | flake8 --diffSummary:
set_freqmethod forDatetimeIndexandTimedeltaIndexfreqsetter forDatetimeIndexandTimedeltaIndex_freqattributeset_freqinstead of the settercc @jreback @jorisvandenbossche