Skip to content

PeriodIndex count & size fix #12874

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

PeriodIndex count & size fix #12874

wants to merge 1 commit into from

Conversation

max-sixty
Copy link
Contributor

closes #12774
closes #12868
closes #12770

  • tests added / passed
  • passes git diff upstream/master | flake8 --diff
  • whatsnew entry

Initial attempt at fixing some of the more urgent issues

@max-sixty max-sixty force-pushed the period-index-resample branch from 413fab3 to f7030b4 Compare April 12, 2016 04:12
@jreback jreback added Bug Period Period data type Resample resample method labels Apr 12, 2016
jreback
jreback reviewed Apr 12, 2016
View reviewed changes
doc/source/whatsnew/v0.18.1.txt
@@ -215,6 +215,9 @@ Bug Fixes
- ``usecols`` parameter in ``pd.read_csv`` is now respected even when the lines of a CSV file are not even (:issue:`12203`)
- Bug in ``groupby.transform(..)`` when ``axis=1`` is specified with a non-monotonic ordered index (:issue:`12713`)
- Bug in ``Period`` and ``PeriodIndex`` creation raises ``KeyError`` if ``freq="Minute"`` is specified. Note that "Minute" freq is deprecated in v0.17.0, and recommended to use ``freq="T"`` instead (:issue:`11854`)
- Bug in ``PeriodIndex.resample`` ``count`` (:issue:`12774`)
Copy link
Contributor

Choose a reason for hiding this comment

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

more of an explanation for the count issue

@max-sixty max-sixty mentioned this pull request Apr 12, 2016
Closed
4 tasks
@max-sixty max-sixty force-pushed the period-index-resample branch 2 times, most recently from b95c188 to fe1fece Compare April 12, 2016 18:01
max-sixty
max-sixty reviewed Apr 12, 2016
View reviewed changes
pandas/tseries/resample.py

return period_range(start, end, **ax_attrs)
return ax._shallow_copy(values, freq=self.freq)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Delegating the attribute consistency to _shallow_copy here

Copy link
Contributor

Choose a reason for hiding this comment

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

yes. (of course be sure to add tests as neeeded, more are better!) and I know you have tests!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha!

This was actually tested in #12771, this is making it more panda-ntic

Copy link
Contributor

Choose a reason for hiding this comment

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

I like panda-ntic! (usually I use pandonic)

@max-sixty max-sixty force-pushed the period-index-resample branch from fe1fece to a0fffb6 Compare April 12, 2016 18:35
@max-sixty max-sixty mentioned this pull request Apr 12, 2016
Closed
@jreback jreback mentioned this pull request Apr 12, 2016
Closed
13 tasks
jreback
jreback reviewed Apr 12, 2016
View reviewed changes
pandas/tseries/resample.py
end = ax[-1].asfreq(self.freq, how='end')
values = []
else:
start = ax[0].asfreq(self.freq, how=self.convention)
Copy link
Contributor

Choose a reason for hiding this comment

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

so this doesn't break the test, I suspect if you did how=self.convention it would work no?
or maybe just ignore convention entirely and make:

start = ax[0].asfreq(self.freq, how='start')
end = ax[-1].asfreq(self.freq, how='end')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the suggestion here, but it breaks that test above. Or am I misunderstanding?

Copy link
Contributor

Choose a reason for hiding this comment

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

actually it I think your change makes sense. make a new sub-section on resampling with PeriodIndex (and put your bug fixes there, with a small example which shows this change).

As an aside, I think convention here is really refering to something completely different. IOW it has to do whether to take the start or end of the original series as the counting point for the freq, NOT the period resampling which is independent of that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless you strongly object I'm going to add an issue on this & follow up; I'm already a bit overextended on this and don't want to leave the stuff completed so far hanging around.

Copy link
Contributor

Choose a reason for hiding this comment

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

no that's fine

@max-sixty max-sixty mentioned this pull request Apr 12, 2016
Open
@max-sixty
Copy link
Contributor Author

@jreback green

@jreback jreback added this to the 0.18.1 milestone Apr 13, 2016
@jreback jreback mentioned this pull request Apr 13, 2016
Closed
@jreback jreback closed this in 77be872 Apr 13, 2016
@jreback
Copy link
Contributor

jreback commented Apr 13, 2016

@MaximilianR thanks!

I used your defs of downsample_methods and such and found a buggie, similar to what you just fixed, e.g. #12886

@gfyoung
Copy link
Member

gfyoung commented Apr 13, 2016

@jreback , @MaximilianR : I'm not sure what happened when this PR was committed to master, but it broke Travis AFAICT (even though the tests here in this PR are passing). Can replicate the failures locally FYI (i.e. no failure on 2.x, but failure on 3.x). Does it have to do with #12886?

jreback added a commit that referenced this pull request Apr 13, 2016
@jreback
TST: revert some testing additions in #12874, xref #12886
2ba977a
@jreback
Copy link
Contributor

jreback commented Apr 13, 2016

I was testing some more things which happened to fail on py3. Reverted for now, the issue is #12866 which is broken.

@gfyoung
Copy link
Member

gfyoung commented Apr 13, 2016

@jreback : I presume you mean #12886? Yeah, I don't quite understand the exceptions caught either that you get with 2.x...confusing... 😕 At least the tests for test_resample.py are all passing again locally 😄

@jreback
Copy link
Contributor

jreback commented Apr 13, 2016

yeah somethings wrong not sure what

@max-sixty
Copy link
Contributor Author

Just saw this. Are there any more details on the breakage?

@jreback
Copy link
Contributor

jreback commented Apr 13, 2016

no this is all fixed though see the linked issue

@max-sixty
Copy link
Contributor Author

OK, thanks

@max-sixty max-sixty deleted the period-index-resample branch May 18, 2016 19:21
yarikoptic added a commit to neurodebian/pandas that referenced this pull request Oct 13, 2016
@yarikoptic
Merge commit 'v0.18.0-114-g6c692ae' into debian
be15f0d 
* commit 'v0.18.0-114-g6c692ae': (492 commits)
  BUG: add names parameter to read_excel
  BUG, DEP, DOC: Patch and Align Categorical's Sorting API
  TST: remove ResourceWarnings from stat by auto-closing iterator
  TST: revert some testing additions in pandas-dev#12874, xref pandas-dev#12886
  DOC: v0.18.1 corrections
  BUG: Respect usecols even with empty data
  PERF: Only do case sensitive check when not lower case
  BUG: PeriodIndex count & resample-on-same-freq fix
  DOC: add windows building blog to contributing.rst
  COMPAT: .query/.eval should work w/o numexpr being installed if possible
  Implement Akima1DInterpolator
  DOC: fix code-block ipython highlighting
  BUG:fixed inconsistent behavior of last_valid_index
  ENH: Add Index.str.get_dummies
  TST: Add more Sparse indexing tests
  ENH: Support CustomBusinessHour
  BUG: ensure coercing scalars to when setting as numpy
  BUG: SparseSeries slicing with Ellipsis raises KeyError
  TST: Add numeric coercion tests
  BUG: .str methods with expand=True may raise ValueError if input has name
  ...
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 Period Period data type Resample resample method
Projects
None yet
Milestone
0.18.1
3 participants
@max-sixty @jreback @gfyoung