!I fix for BUG: resample with tz-aware: Values falls after last bin #15549 #18337

ahcub · 2017-11-17T03:21:38Z

closes BUG: resample with tz-aware: Values falls after last bin #15549
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

… bin

…t bin

…ndas-dev-master

gfyoung · 2017-11-17T23:32:12Z

pandas/tests/test_resample.py

@@ -2719,6 +2719,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')


Reference issue number.

sorry, but I already referenced it in the function name or you mean something else?

ahcub · 2017-11-18T01:37:39Z

guys, can you help me to fix problem with travis?
I'm not sure what is wrong there...

gfyoung · 2017-11-18T01:43:16Z

Read the output of the failing Travis build here. Your code fails to pass style-checks.

The failure on the non-blocking build seems unrelated. Might just need to rebase on master for it to go away on that build.

codecov · 2017-11-19T01:00:15Z

Codecov Report

Merging #18337 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #18337      +/-   ##
==========================================
- Coverage   91.38%   91.34%   -0.04%     
==========================================
  Files         164      164              
  Lines       49790    49794       +4     
==========================================
- Hits        45501    45485      -16     
- Misses       4289     4309      +20

Flag	Coverage Δ
#multiple	`89.14% <100%> (-0.03%)`	⬇️
#single	`39.49% <42.85%> (-0.07%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/indexes/datetimes.py	`95.49% <100%> (+0.01%)`	⬆️
pandas/core/resample.py	`96.33% <100%> (+0.17%)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/plotting/_converter.py	`63.44% <0%> (-1.82%)`	⬇️
pandas/core/frame.py	`97.8% <0%> (-0.1%)`	⬇️

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 774030c...fc795c6. Read the comment docs.

codecov · 2017-11-19T01:00:16Z

Codecov Report

Merging #18337 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #18337      +/-   ##
==========================================
- Coverage   91.36%   91.32%   -0.05%     
==========================================
  Files         164      164              
  Lines       49718    49733      +15     
==========================================
- Hits        45426    45418       -8     
- Misses       4292     4315      +23

Flag	Coverage Δ
#multiple	`89.12% <100%> (-0.03%)`	⬇️
#single	`39.62% <0%> (-0.05%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/resample.py	`96.34% <100%> (+0.18%)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/plotting/_converter.py	`63.44% <0%> (-1.82%)`	⬇️
pandas/core/series.py	`94.84% <0%> (-0.19%)`	⬇️
pandas/core/frame.py	`97.8% <0%> (-0.1%)`	⬇️
pandas/core/internals.py	`94.47% <0%> (-0.08%)`	⬇️
pandas/core/indexes/base.py	`96.43% <0%> (ø)`	⬆️
pandas/core/indexes/datetimes.py	`95.48% <0%> (ø)`	⬆️
pandas/core/indexes/period.py	`92.92% <0%> (+0.02%)`	⬆️
pandas/plotting/_core.py	`82.49% <0%> (+0.03%)`	⬆️
... and 2 more

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 1647a72...2e77f72. Read the comment docs.

jreback · 2017-11-19T15:01:42Z

pandas/core/indexes/datetimes.py

@@ -366,9 +366,11 @@ def __new__(cls, data=None,
                pass

        if data is None:
+            values_present = kwargs.pop('values_present', False)


what the heck is all of this?

sorry, what do you mean?

that is a change to fix a problem with resampling for tz-aware series in one of the edge cases
I hit this problem often unfortunately and from my understanding of the design I created a solution that doesn't break other functionality but fixes the problem I have
sorry if the solution doesn't look elegant, if you think there is other one that we can use, please let me know

I am not really sure what you are doing. Your changes should be restriced to pandas/core/resample.py

if you mean that I should not change anything in pandas/core/resample.py then unfortunately I don't know how to avoid that

I mean the opposite. There's where the change should be.

I can implement a solution with changes in pandas/core/resample.py only I think
let me try

yeah I think this is a specific change that resample needs, not generically for date_ranges.

jreback · 2017-11-19T16:49:47Z

pandas/core/indexes/datetimes.py

@@ -366,9 +366,11 @@ def __new__(cls, data=None,
                pass

        if data is None:
+            values_present = kwargs.pop('values_present', False)


I am not really sure what you are doing. Your changes should be restriced to pandas/core/resample.py

jreback · 2017-11-19T16:50:45Z

pandas/core/resample.py

@@ -1139,7 +1142,8 @@ def _get_time_bins(self, ax):
                                        start=first,


what exactly are you trying to do here? what is the purpose of values_present, IOW what case are you trying to handle?

the case is described here #15549
unfortunately, I get values fall after last bin error when I do resampling of tz-aware series with summer-winter time change

so in case if we are resampling series I believe an extra bin should be generated to handle this
but in case of generation of the datetimeindex with date_range for example we should not get the extra bin

purpose of values_present is to be able to say which case is that, that we are creating datetimeindex for

hope that makes sense

I understand the case. My point is that you can detect what you need and simply generate different bins at that point, rather than intrude to the implementation of date_range.

the soln that you posted in the issue looks promising. did you try that?

yep, last time I made a PR with it you said that it is not right to resample by UTC a tz aware series
and after giving it a thought I think you are right
it's not a good idea to resample everything by UTC, also I did that change internally for our company and I got issues with it.
like when I resample with B freq, I get a data point on sunday, but not on friday

yeah that might be a bigger change, ok (and maybe not what we want semantically)

jreback · 2017-11-19T21:07:49Z

pandas/core/resample.py

+                                        name=ax.name)
+
+        # GH 15549
+        values_present = isinstance(getattr(self, 'obj', None),


still not clear why you need this tests for obj is Series/DataFrame. that 's all it can be.

also need to check that len(binner) >= 2

ok, I was not sure about that the obj always exists
I will do the check

there was actually a mistake, it supposed to go start from binner[-1] not binner[-2], I fixed it anyway

ahcub · 2017-11-20T07:38:06Z

@jreback please check the change, it looks like the most elegant version so far

jreback

looks good. some comments. pls add a whatsnew for 0.21.1 bug fix.

jreback · 2017-11-20T11:32:25Z

pandas/core/resample.py

@@ -1141,6 +1141,13 @@ def _get_time_bins(self, ax):
                                        tz=tz,
                                        name=ax.name)

+        # GH 15549
+        if len(binner) > 1 and binner[-1] < last:


can you add a comment here on what is going on.

jreback · 2017-11-20T11:32:36Z

pandas/tests/test_resample.py

@@ -2719,6 +2719,12 @@ 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],


move the issue number to a comment (and out of the title)

jreback · 2017-11-20T11:32:56Z

pandas/tests/test_resample.py

+        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()


add an expected and compare using assert_frame_equal

pep8speaks · 2017-11-20T16:44:44Z

Hello @ahcub! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on November 21, 2017 at 07:36 Hours UTC

ahcub · 2017-11-21T00:15:02Z

@jreback I think the changes you requested were made, please let me know if anything else needs to be made or if all good

jreback

lgtm. small comments
ping on green

jreback · 2017-11-21T00:52:40Z

doc/source/whatsnew/v0.21.1.txt

@@ -62,6 +62,7 @@ Bug Fixes
 - Bug in ``pd.Series.rolling.skew()`` and ``rolling.kurt()`` with all equal values has floating issue (:issue:`18044`)
 - Bug in ``pd.DataFrameGroupBy.count()`` when counting over a datetimelike column (:issue:`13393`)
 - Bug in ``pd.concat`` when empty and non-empty DataFrames or Series are concatenated (:issue:`18178` :issue:`18187`)
+- Bug in ``DataFrame.resample(...)`` when there is a time change, resampling frequecy is big enough (:issue:`15549`)


say DST here

sp on frequency

jreback · 2017-11-21T00:54:23Z

pandas/core/resample.py

+        # GH 15549
+        # In edge case of tz-aware resapmling binner last index can be
+        # less than the last variable in data object.
+        # This leads to `Values falls after last bin` error


this last sentence is not relevant; you are fixing this!
add why the bonner is too short (eg because of DST)

jreback · 2017-11-21T00:55:25Z

pandas/tests/test_resample.py

+        index = pd.DatetimeIndex([1457537600000000000, 1458059600000000000],
+                                 tz='UTC').tz_convert('America/Chicago')
+        df = pd.DataFrame([1, 2], index=index)
+        res_df = df.resample('12h', closed='right',


call this result

jreback · 2017-11-21T00:55:42Z

pandas/tests/test_resample.py

+                                 '2016-03-15 01:00:00-05:00',
+                                 '2016-03-15 13:00:00-05:00']
+        index = pd.DatetimeIndex(expected_index_values,
+                                 tz='UTC').tz_convert('America/Chicago')


call this expected

jreback · 2017-11-21T00:56:26Z

pandas/tests/test_resample.py

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

+    def test_resample_tz_aware(self):


be more explicit in the test name
this is particularl to dst

ahcub · 2017-11-21T01:47:01Z

did the changes, let me know if anything else is needed to change

ahcub · 2017-11-21T08:52:56Z

@jreback looks like it is green now
let me know if anything else needs to be changed

jreback · 2017-11-21T13:13:18Z

this might in theory fix:

#6085 and #12351

jreback · 2017-11-21T13:14:17Z

thanks!

…andas-dev#15549 (pandas-dev#18337) (cherry picked from commit 8efd1a0)

…15549 (#18337) (cherry picked from commit 8efd1a0)

ahcub added 13 commits March 2, 2017 22:27

!B [pandas-dev#15549] resample with tz-aware: Values falls after last…

68a02aa

… bin

restore mistakenly removed line

47b99ac

remove redundant whitespaces

04ce929

remove redundant whitespaces

a43bb19

!U add tz to DatetimeIndex initialization

44fc3f2

!U add test for a bug: resample with tz-aware: Values falls after las…

2a1d24c

…t bin

!U do not convert TZ to UTC if it not set

d28ff98

!U revert changes with timezone

7c770c0

!U revert spaces

effa650

!U change way the end timestamp is defined

aa8c0af

Merge branch 'master' of https://github.com/pandas-dev/pandas into pa…

2db6576

…ndas-dev-master

Merge branch 'pandas-dev-master'

2744ad3

!U add conditional change in the formula for defining the end timestamp

fb4d405

gfyoung added Bug Resample resample method labels Nov 17, 2017

gfyoung reviewed Nov 17, 2017

View reviewed changes

!U make the values_present argument optional

93842d0

!U add line feed

fc795c6

ahcub added 3 commits November 18, 2017 17:01

!U fix formatting

514fe24

!U add a line feed

777a12a

!U fix formatting

037c9dc

jreback requested changes Nov 19, 2017

View reviewed changes

ahcub added 3 commits November 19, 2017 12:58

!U change the fix to only handle the problem on core/resample.py level

d5ac67e

!U fix formatting

61e84d6

!U change the index from which we get the dates to appent

e049112

jreback requested changes Nov 19, 2017

View reviewed changes

ahcub added 3 commits November 19, 2017 13:10

!U remove redundant check and fix index

e252b20

!U add check for binner len

1888355

!U add missing arguments to the extra dates generation

f4ed7c0

jreback requested changes Nov 20, 2017

View reviewed changes

ahcub added 2 commits November 20, 2017 08:26

!U add comments

9b18f4b

!U change test name and add expected df to compare result to

5b035f0

ahcub added 4 commits November 20, 2017 08:47

!R reformat the code

0ec3bd0

!U add whatsnew bug fix entry

b523915

!U reformat the code

a1b59c4

Merge branch 'master' into master

8cd36c9

ahcub closed this Nov 20, 2017

ahcub reopened this Nov 20, 2017

jreback requested changes Nov 21, 2017

View reviewed changes

!U reformat the code according to PR comments

aec765f

!U fix formatting

2e77f72

jreback added this to the 0.21.1 milestone Nov 21, 2017

jreback approved these changes Nov 21, 2017

View reviewed changes

jreback merged commit 8efd1a0 into pandas-dev:master Nov 21, 2017

TomAugspurger pushed a commit to TomAugspurger/pandas that referenced this pull request Dec 8, 2017

!I fix for BUG: resample with tz-aware: Values falls after last bin p…

57c24a3

…andas-dev#15549 (pandas-dev#18337) (cherry picked from commit 8efd1a0)

TomAugspurger pushed a commit that referenced this pull request Dec 11, 2017

!I fix for BUG: resample with tz-aware: Values falls after last bin #…

014b8d6

…15549 (#18337) (cherry picked from commit 8efd1a0)

		@@ -1139,7 +1142,8 @@ def _get_time_bins(self, ax):
		start=first,

!I fix for BUG: resample with tz-aware: Values falls after last bin #15549 #18337

!I fix for BUG: resample with tz-aware: Values falls after last bin #15549 #18337

Conversation

ahcub commented Nov 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahcub commented Nov 18, 2017 • edited Loading

gfyoung commented Nov 18, 2017 • edited Loading

codecov bot commented Nov 19, 2017

Codecov Report

codecov bot commented Nov 19, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahcub Nov 19, 2017 • edited Loading

Choose a reason for hiding this comment

jreback Nov 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahcub commented Nov 20, 2017

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pep8speaks commented Nov 20, 2017 • edited Loading

Comment last updated on November 21, 2017 at 07:36 Hours UTC

ahcub commented Nov 21, 2017

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahcub commented Nov 21, 2017

ahcub commented Nov 21, 2017

jreback commented Nov 21, 2017

jreback commented Nov 21, 2017

ahcub commented Nov 18, 2017 •

edited

Loading

gfyoung commented Nov 18, 2017 •

edited

Loading

codecov bot commented Nov 19, 2017 •

edited

Loading

ahcub Nov 19, 2017 •

edited

Loading

jreback Nov 19, 2017 •

edited

Loading

pep8speaks commented Nov 20, 2017 •

edited

Loading