Skip to content

Retain name in PeriodIndex resample #12771

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Retain name in PeriodIndex resample #12771

wants to merge 1 commit into from

Conversation

max-sixty
Copy link
Contributor

@max-sixty max-sixty force-pushed the retain-period-index-name-on-resample branch 2 times, most recently from 438b29c to 8ecd0ef Compare April 2, 2016 00:48
max-sixty
max-sixty reviewed Apr 2, 2016
View reviewed changes
pandas/tseries/resample.py
@@ -652,9 +652,6 @@ def _convert_obj(self, obj):
# Cannot have multiple of periods, convert to timestamp
self.kind = 'timestamp'

if not len(obj):
self.kind = 'timestamp'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what the goal of this was. It made this test I added fail

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the kind argument is quite tricky. We really should remove it, but that's a whole other project. The issue is that some of the routines are expecting DatetimeIndex so we give it to them even though we are a PeriodIndexResampelr. This all existed before the changed in 0.18.0, but was much trickier to deal with as it was function based. So should be possible to fix now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am going to punt this to #12774, and loosen the associated test in this PR

@jreback
Copy link
Contributor

jreback commented Apr 2, 2016

pls don't change stuff just because py charm does
use targeted changes

@max-sixty
Copy link
Contributor Author

I would remove it if I didn't think it made it better / cleaner. Happy to if you don't think it does...

@jreback jreback added Bug Indexing Related to indexing on series/frames, not to indexes themselves Period Period data type Resample resample method labels Apr 2, 2016
jreback
jreback reviewed Apr 2, 2016
View reviewed changes
doc/source/whatsnew/v0.18.1.txt
@@ -192,7 +192,7 @@ Bug Fixes


- Bug in ``CategoricalIndex.get_loc`` returns different result from regular ``Index`` (:issue:`12531`)

- Bug in ``PeriodIndex.resample`` where name not propagated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add the issue number

@max-sixty max-sixty force-pushed the retain-period-index-name-on-resample branch 2 times, most recently from cf1b05b to 43ddeb2 Compare April 2, 2016 16:55
@jreback
Copy link
Contributor

jreback commented Apr 2, 2016

run git diff master | flake8 --diff to see PEP issues

@max-sixty max-sixty force-pushed the retain-period-index-name-on-resample branch from 43ddeb2 to 3dbcba6 Compare April 2, 2016 22:22
@max-sixty
Copy link
Contributor Author

@jreback green

@max-sixty
Copy link
Contributor Author

Although codecov states that this line isn't tested:
image

Unless I'm mistaken, I added a test for this + a note that the test needed to be expanded to test for the index, rather than only its length. But that requires another PR

jreback
jreback reviewed Apr 2, 2016
View reviewed changes
pandas/tseries/tests/test_timeseries.py
result = s.resample('M').sum().index

# would like to test vs the actual index, but due to GH12774,
# currently only testing the length
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no create the expected Series as it comes back
you can fix it later in the other issue

@jreback
Copy link
Contributor

jreback commented Apr 2, 2016

no I just fixed something on codecov

@max-sixty max-sixty force-pushed the retain-period-index-name-on-resample branch from 3dbcba6 to 7b17a7e Compare April 2, 2016 23:58
jreback
jreback reviewed Apr 3, 2016
View reviewed changes
pandas/tseries/tests/test_timeseries.py
index = PeriodIndex(start='2000', periods=0, freq='D', name='idx')
s = Series(index=index)
result = s.resample('M').sum()
# after GH12774 is resolved, this should be a PeriodIndex
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great. put a pointer to this PR in that issue (and maybe a checkbox), so we don't forget.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@jreback jreback added this to the 0.18.1 milestone Apr 3, 2016
@jreback
Copy link
Contributor

jreback commented Apr 3, 2016

looks good. ping on green.

@max-sixty
Copy link
Contributor Author

@jreback green, apart from the codecov anomaly

@jreback jreback closed this in 05b2189 Apr 3, 2016
@jreback
Copy link
Contributor

jreback commented Apr 3, 2016

thanks @MaximilianR

I moved the tests to the proper location (and some other ones, not sure why there were there). test_resample.

now, if in test_resample you do this (and do it with _ts as well).

def _simple_pts(start, end, freq='D'):
    rng = period_range(start, end, freq=freq, name='foo')
    return Series(np.random.randn(len(rng)), index=rng)

you will get some failures, could just be some test failures though (e.g. the expected need to change).

@max-sixty max-sixty deleted the retain-period-index-name-on-resample branch April 3, 2016 21:05
@max-sixty max-sixty mentioned this pull request Apr 12, 2016
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Bug Indexing Related to indexing on series/frames, not to indexes themselves Period Period data type Resample resample method
Projects
None yet
Milestone
0.18.1
Development

Successfully merging this pull request may close these issues.

BUG: Resample loses PeriodIndex name
2 participants
@max-sixty @jreback