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

BUG: resample with tz-aware: Values falls after last bin #15555

Closed
wants to merge 7 commits into from

Conversation

ahcub
Copy link
Contributor

@ahcub ahcub commented Mar 2, 2017

  • closes BUG: resample with tz-aware: Values falls after last bin #15549
  • tests added / passed
  • passes git diff upstream/master | flake8 --diff
  • whatsnew entry: resampling will now always produce bins alligned by UTC instead of custom time zone. so for example if you resampling by 12 hours frequency in UTC-5 timezone, you will get bins like
    7:00 19:00 7:00 19:00
    instead of
    00:00 12:00 00:00 12:00

@ahcub
Copy link
Contributor Author

ahcub commented Mar 2, 2017

jreback, would you like me to add tests for this change ?

@jreback
Copy link
Contributor

jreback commented Mar 2, 2017

tests!

always

and a whatsnew note please.

@jreback jreback changed the title !B [pandas-dev/pandas#15549] resample with tz-aware: Values falls after last bin BUG: resample with tz-aware: Values falls after last bin Mar 2, 2017
@jreback jreback added Bug Resample resample method labels Mar 2, 2017
@ahcub
Copy link
Contributor Author

ahcub commented Mar 3, 2017

committed a test and wrote a whatsnew note.
please let me know if the format of whatsnew is correct, I'm not sure if there is any standard form I should follow.

@ahcub
Copy link
Contributor Author

ahcub commented Mar 3, 2017

I understand why the checks failed, I will commit a fix

@ahcub
Copy link
Contributor Author

ahcub commented Mar 4, 2017

@jreback is it ok to change already existing tests in order to make my change pass them ?

@jreback
Copy link
Contributor

jreback commented Mar 4, 2017

you need to prove that a current test is wrong in order to change it

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.

I am not so sure I buy the rationale for the change here. The binning MUST be in the local tz, and NOT UTC, otherwise that is VERY confusing (IOW you would have really odd breakpoints which have nothing to do with the data, but the timezone). T

def test_resample_tz_aware_bug_15549(self):
index = pd.DatetimeIndex([1450137600000000000, 1474059600000000000], tz='UTC').tz_convert('America/Chicago')
df = pd.DataFrame([1, 2], index=index)
df.resample('12h', closed='right', label='right').last().ffill()
Copy link
Contributor

Choose a reason for hiding this comment

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

result = df.resample(.......)
expected = ....
tm.assert_frame_equal(result, expected)

add a comment with the issue number as a comment (and not in the test name)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree, I will fix it. I just used the same notation as in the test above. thought it's a common thing

@@ -2670,6 +2670,11 @@ def test_resample_weekly_bug_1726(self):
# it works!
df.resample('W-MON', closed='left', label='left').first()

def test_resample_tz_aware_bug_15549(self):
index = pd.DatetimeIndex([1450137600000000000, 1474059600000000000], tz='UTC').tz_convert('America/Chicago')
Copy link
Contributor

Choose a reason for hiding this comment

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

this line is going to be too long for linting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

understood, I will change it

@jreback
Copy link
Contributor

jreback commented Mar 4, 2017

I think you need to compute the range edges with the tz intact. I think it is mishandling when it has an ambiguous datetime.

Then constructing the tz should be ok.

@ahcub
Copy link
Contributor Author

ahcub commented Mar 6, 2017

got it, let me try to use your approach

@jreback
Copy link
Contributor

jreback commented Mar 6, 2017

@ahcub yeah this seemingly trivial change is actually hard. there have been lots of fixes (for various reasons) thru the years. good news is the test suite is quite comprehensive here :>

@jreback
Copy link
Contributor

jreback commented Mar 28, 2017

can you rebase / update

@ahcub
Copy link
Contributor Author

ahcub commented Mar 28, 2017

I will close it for now and restore it when I get back to it

@ahcub ahcub closed this Mar 28, 2017
@jreback
Copy link
Contributor

jreback commented Mar 28, 2017

@ahcub ok sure. though pls come back when you can! thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Resample resample method
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants