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

Continue porting period_helper; fix leftover asfreq bug #19834

Merged
merged 5 commits into from Feb 23, 2018

Conversation

Projects
None yet
2 participants
@jbrockmendel
Member

jbrockmendel commented Feb 22, 2018

In #19650 we fixed a bug with asfreq but missed a weekly-frequency case. This fixes and tests that. I'll see if I can cook up a more thorough set of tests to make sure nothing else slips through the cracks.

Other than that, the main thing this does is substitute in ORD_OFFSET = WEEK_OFFSET * 7 + 4 and BDAY_OFFSET = 5 * WEEK_OFFSET + 4 and then cleans up some algebra (which is now OK because we are using non-cdivision floordiv and mod). As a result we get rid of a bunch of calls that add ORD_OFFSET just to subtract it a few lines later.

Gets rid of CamelCase in tslibs.period, moves towards using pandas_datetimestruct directly and getting rid of the redundant date_info.

Catches a RuntimeWarning in the tests; closes #19767.

@jreback

ok except for the catching the RunTimewarning on localize. easiest prob to remove and do that next pass

@@ -28,15 +28,17 @@ def test_tz_localize_pushes_out_of_bounds(self):
pac = Timestamp.min.tz_localize('US/Pacific')
assert pac.value > Timestamp.min.value
pac.tz_convert('Asia/Tokyo') # tz_convert doesn't change value
with pytest.raises(OutOfBoundsDatetime):
Timestamp.min.tz_localize('Asia/Tokyo')
with tm.assert_produces_warning(RuntimeWarning):

This comment has been minimized.

@jreback

jreback Feb 22, 2018

Contributor

I think there is a missing check in localize itself, IOW need to check if something is 0

@@ -21,6 +21,16 @@ def test_asfreq_near_zero(self, freq):
tup2 = (prev.year, prev.month, prev.day)
assert tup2 < tup1
def test_asfreq_near_zero_weekly(self):

This comment has been minimized.

@jreback

jreback Feb 22, 2018

Contributor

can rename to test_asfreq?

This comment has been minimized.

@jbrockmendel

jbrockmendel Feb 22, 2018

Member

Name follows convention set by the test above it (test_asfreq_near_zero)

This comment has been minimized.

@jreback

jreback Feb 23, 2018

Contributor

I mean the module name itself.

@jbrockmendel

This comment has been minimized.

Member

jbrockmendel commented Feb 22, 2018

easiest prob to remove and do that next pass

sure

@codecov

This comment has been minimized.

codecov bot commented Feb 22, 2018

Codecov Report

Merging #19834 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #19834   +/-   ##
=======================================
  Coverage   91.58%   91.58%           
=======================================
  Files         150      150           
  Lines       48890    48890           
=======================================
  Hits        44775    44775           
  Misses       4115     4115
Flag Coverage Δ
#multiple 89.95% <ø> (ø) ⬆️
#single 41.78% <ø> (ø) ⬆️

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 a6183a2...3123c0e. Read the comment docs.

@jreback

rename, ping on green.

@@ -21,6 +21,16 @@ def test_asfreq_near_zero(self, freq):
tup2 = (prev.year, prev.month, prev.day)
assert tup2 < tup1
def test_asfreq_near_zero_weekly(self):

This comment has been minimized.

@jreback

jreback Feb 23, 2018

Contributor

I mean the module name itself.

@jreback jreback added this to the 0.23.0 milestone Feb 23, 2018

@jbrockmendel

This comment has been minimized.

Member

jbrockmendel commented Feb 23, 2018

I mean the module name itself.

That makes much more sense. Mind waiting for the next pass since appveyor has a 12hour+ turnaround?

@jreback

This comment has been minimized.

Contributor

jreback commented Feb 23, 2018

That makes much more sense. Mind waiting for the next pass since appveyor has a 12hour+ turnaround?

ok

@jreback jreback merged commit c3e35a0 into pandas-dev:master Feb 23, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jbrockmendel jbrockmendel deleted the jbrockmendel:phelper15 branch Feb 23, 2018

harisbal pushed a commit to harisbal/pandas that referenced this pull request Feb 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment