-
-
Notifications
You must be signed in to change notification settings - Fork 19.3k
ENH: PeriodIndex can accept freq with mult #7832
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
Conversation
|
I think this might need an example in the docs as well http://pandas.pydata.org/pandas-docs/stable/timeseries.html#periodindex-and-period-range (you can use same example in release notes) |
|
this is a bit odd though when you operate on these: you are not propogating the given freq to the actual period? |
|
@sinhrks can you rebase |
|
@sinhrks can you respond to my comments above? |
|
@sinhrks ? |
240873a to
677eb75
Compare
|
@sinhrks can you rebase / revisit |
|
@jreback Indeed, the frequency given to In [11]: pd.period_range('2-1-2014 00:00', '2-1-2014 3:00', freq='30Min').tolist()
Out[11]:
[Period('2014-02-01 00:00', 'T'),
Period('2014-02-01 00:30', 'T'),
Period('2014-02-01 01:00', 'T'),
Period('2014-02-01 01:30', 'T'),
Period('2014-02-01 02:00', 'T'),
Period('2014-02-01 02:30', 'T'),
Period('2014-02-01 03:00', 'T')] |
|
@blbradley yes that sounds right. When the periods are created during iteration (e.g. the It should also be true when this PR is implementd. |
|
@jreback Thanks. I started trying to implement that. We'll see how it goes. |
|
@jreback @blbradley OK. In #7811, I initially thought that created |
|
|
|
@blbradley this is true. But that's a whole separate project (though not THAT difficult). feel free! |
|
@sinhrks I've been revisiting this. One problem is that |
|
@sinhrks I would love to collaborate on this. Please respond soon cause I'm actively working on it. |
|
@sinhrks ? |
|
@jreback This year sometime! I did some code spikes towards it before, but they were not fruitful. |
|
Maybe first change |
d85b101 to
408ac66
Compare
|
OK, fixed to raise when |
|
Maybe the whatsnew notice can give it a bit more attention? (eg an example). |
|
I can't spot anything else that would stop this going in. |
e5a8ddc to
54c92b8
Compare
|
@jorisvandenbossche Yes, updated whatsnew and timeseries doc. @MaximilianR I think this has been removed from |
|
@sinhrks you're right - something funky was going on with my extensions - thanks |
doc/source/whatsnew/v0.17.0.txt
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's put this in a separate sub-section
|
@sinhrks lgtm. couple of comments. merge after fix and green. |
|
@jreback OK, fixed the release note and updated pickle/msgpack. |
|
ok, pls rebase then merge |
|
Rebased and now green. Merging today (Sep 3) if no further comments. |
|
lgtm. merge away! |
ENH: PeriodIndex can accept freq with mult
|
thanks @sinhrks awesome effort! merging as I need to rebase tz on top of this |
|
Thanks for review and merge:) |
Closes #7811.
Period.freqandPeriodIndex.freqto store offsets.freqstrtoPeriod,PeriodIndexcan useDatetimeIndexOpsMixin's logicnproeprties (BUG: frequencies.get_freq_code raises an error against offset with n != 1 #10350)asfreqandto_timestampusing freq with mult.PeriodIndex)Decide a policy for legacy freq aliases, like "WK"(handled in DEPR: deprecate legacy offsets #10878)