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

ENH: PeriodIndex can accept freq with mult #7832

Merged
merged 1 commit into from
Sep 3, 2015

Conversation

sinhrks
Copy link
Member

@sinhrks sinhrks commented Jul 24, 2014

Closes #7811.

@jreback jreback added this to the 0.15.0 milestone Jul 24, 2014
@jreback
Copy link
Contributor

jreback commented Jul 24, 2014

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)

@jreback
Copy link
Contributor

jreback commented Jul 24, 2014

this is a bit odd though when you operate on these:

you are not propogating the given freq to the actual period?

In [10]: PeriodIndex(start='2014-01', freq='3M', periods=4).tolist()
Out[10]: 
[Period('2014-01', 'M'),
 Period('2014-04', 'M'),
 Period('2014-07', 'M'),
 Period('2014-10', 'M')]

In [11]: (PeriodIndex(start='2014-01', freq='3M', periods=4) + 1).tolist()
Out[11]: 
[Period('2014-02', 'M'),
 Period('2014-05', 'M'),
 Period('2014-08', 'M'),
 Period('2014-11', 'M')]

@jreback
Copy link
Contributor

jreback commented Sep 8, 2014

@sinhrks can you rebase

@jreback
Copy link
Contributor

jreback commented Sep 14, 2014

@sinhrks can you respond to my comments above?

@jreback
Copy link
Contributor

jreback commented Sep 19, 2014

@sinhrks ?

@jreback jreback modified the milestones: 0.15.1, 0.15.0 Sep 26, 2014
@sinhrks sinhrks force-pushed the period_mult branch 2 times, most recently from 240873a to 677eb75 Compare November 22, 2014 10:15
@jreback
Copy link
Contributor

jreback commented Jan 18, 2015

@sinhrks can you rebase / revisit

@blbradley
Copy link
Contributor

@jreback Indeed, the frequency given to PeriodIndex is not propagated to each Period within. Do you think it should be propagated? Your earlier response indicates that you do, but I'm checking in to verify.
This unwanted behavior is also shown using period_range:

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')]

@jreback
Copy link
Contributor

jreback commented Jan 30, 2015

@blbradley yes that sounds right. When the periods are created during iteration (e.g. the tolist()) then the freq should (and currently is passed).

It should also be true when this PR is implementd.

@blbradley
Copy link
Contributor

@jreback Thanks. I started trying to implement that. We'll see how it goes.

@sinhrks
Copy link
Member Author

sinhrks commented Jan 31, 2015

@jreback @blbradley OK. In #7811, I initially thought that created PeriodIndex doesn't need custom freq, but it is more natural. I'll consider the implementation.

@blbradley
Copy link
Contributor

Period would greatly benefit from being rewritten in Cython (probably as part of tslib) as to take advantage of convert_to_tsobject.

@jreback
Copy link
Contributor

jreback commented Jan 31, 2015

@blbradley this is true. But that's a whole separate project (though not THAT difficult). feel free!

@blbradley
Copy link
Contributor

@sinhrks I've been revisiting this. One problem is that Period.asfreq does not handle multiples. I think this could be handled by creating a PeriodIndex the same way as DatetimeIndex in pandas.tseries.resample.asfreq.

@blbradley
Copy link
Contributor

@sinhrks I would love to collaborate on this. Please respond soon cause I'm actively working on it.

@jreback jreback modified the milestones: 0.16.1, 0.16.0 Mar 5, 2015
@jreback
Copy link
Contributor

jreback commented Apr 8, 2015

@sinhrks ?
@blbradley interested in working on this one?

@blbradley
Copy link
Contributor

@jreback This year sometime! I did some code spikes towards it before, but they were not fruitful.

@sinhrks
Copy link
Member Author

sinhrks commented Apr 9, 2015

Maybe first change Period to take multiple freqs, then revisit this. I'll look into Period code.

@jreback jreback modified the milestones: 0.17.0, 0.16.1 Apr 29, 2015
@sinhrks sinhrks force-pushed the period_mult branch 2 times, most recently from d85b101 to 408ac66 Compare August 24, 2015 13:17
@sinhrks
Copy link
Member Author

sinhrks commented Aug 24, 2015

OK, fixed to raise when freq is not positive, and added some tests. Ready for review once again.

@jorisvandenbossche
Copy link
Member

Maybe the whatsnew notice can give it a bit more attention? (eg an example).
Further, are there any API changes to note as well? (that freq now returns an Offset object instead of string?)

@max-sixty
Copy link
Contributor

I can't spot anything else that would stop this going in.
Not sure if my comment on the docstring is wrong or it just got missed - it still states that you can't provide 5T etc

    e.g., 'B' for businessday. Must be a singular rule-code (e.g. 5T is not
    allowed).

@sinhrks sinhrks force-pushed the period_mult branch 5 times, most recently from e5a8ddc to 54c92b8 Compare August 26, 2015 13:38
@sinhrks
Copy link
Member Author

sinhrks commented Aug 26, 2015

@jorisvandenbossche Yes, updated whatsnew and timeseries doc.

@MaximilianR I think this has been removed from period.pyx after your suggestion. If I miss other remaining, please point the line.

@max-sixty
Copy link
Contributor

@sinhrks you're right - something funky was going on with my extensions - thanks

@@ -177,7 +177,28 @@ Other enhancements
- ``pandas.tseries.offsets`` larger than the ``Day`` offset can now be used with with ``Series`` for addition/subtraction (:issue:`10699`). See the :ref:`Documentation <timeseries.offsetseries>` for more details.

- ``.as_blocks`` will now take a ``copy`` optional argument to return a copy of the data, default is to copy (no change in behavior from prior versions), (:issue:`9607`)
- ``Period``, ``PeriodIndex`` and ``period_range`` can now accept multiplied freq (:issue:`7811`)
Copy link
Contributor

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

@jreback
Copy link
Contributor

jreback commented Aug 31, 2015

@sinhrks lgtm. couple of comments. merge after fix and green.

@sinhrks
Copy link
Member Author

sinhrks commented Aug 31, 2015

@jreback OK, fixed the release note and updated pickle/msgpack.

@jreback
Copy link
Contributor

jreback commented Sep 1, 2015

ok, pls rebase then merge

@sinhrks
Copy link
Member Author

sinhrks commented Sep 2, 2015

Rebased and now green. Merging today (Sep 3) if no further comments.

@jreback
Copy link
Contributor

jreback commented Sep 2, 2015

lgtm. merge away!

jreback added a commit that referenced this pull request Sep 3, 2015
ENH: PeriodIndex can accept freq with mult
@jreback jreback merged commit 8aeaf02 into pandas-dev:master Sep 3, 2015
@jreback
Copy link
Contributor

jreback commented Sep 3, 2015

thanks @sinhrks awesome effort!

merging as I need to rebase tz on top of this

@sinhrks
Copy link
Member Author

sinhrks commented Sep 3, 2015

Thanks for review and merge:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Frequency DateOffsets Period Period data type
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ENH: Allow period_range to accept freq with mult != 1
6 participants