Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Nov 27, 2018

@pep8speaks
Copy link

pep8speaks commented Nov 27, 2018

Hello @ArtinSarraf! Thanks for updating the PR.

Comment last updated on December 08, 2018 at 05:00 Hours UTC

@codecov
Copy link

codecov bot commented Nov 28, 2018

Codecov Report

Merging #23941 into master will decrease coverage by <.01%.
The diff coverage is 91.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #23941      +/-   ##
==========================================
- Coverage   92.22%   92.22%   -0.01%     
==========================================
  Files         162      162              
  Lines       51785    51805      +20     
==========================================
+ Hits        47758    47776      +18     
- Misses       4027     4029       +2
Flag Coverage Δ
#multiple 90.62% <91.3%> (-0.01%) ⬇️
#single 42.99% <8.69%> (-0.02%) ⬇️
Impacted Files Coverage Δ
pandas/core/resample.py 96.58% <91.3%> (-0.24%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c037128...c234cac. Read the comment docs.

@gfyoung gfyoung added Bug Datetime Datetime data dtype Resample resample method labels Nov 28, 2018
Copy link
Member

@gfyoung gfyoung left a comment

Choose a reason for hiding this comment

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

Good start! Also needs a whatsnew entry.

j = -1 if self.freq.onOffset(end) else None

labels = binner = PeriodIndex(start=p_start, end=p_end,
freq=self.freq,
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a lint error

@ghost
Copy link
Author

ghost commented Dec 4, 2018

@jreback - caught a bug in my original bin offset calculation. Previously it was only valid when the index started at the 0 base of the index's frequency. I've fixed that and added several test cases that previously would have failed.

@jreback
Copy link
Contributor

jreback commented Dec 5, 2018

can you merge master

@ghost
Copy link
Author

ghost commented Dec 8, 2018

@mroeschke - reverted the changes here and added a new PR.
#24159

@ghost
Copy link
Author

ghost commented Dec 9, 2018

@jreback / @mroeschke - anything else to consider?

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

looks good. just want some more docs.

@jreback jreback added this to the 0.24.0 milestone Dec 13, 2018
@jreback jreback merged commit 040f06f into pandas-dev:master Dec 14, 2018
@jreback
Copy link
Contributor

jreback commented Dec 14, 2018

thanks @ArtinSarraf

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Datetime Datetime data dtype Resample resample method

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BUG: base argument ignored in PeriodIndex resample

4 participants