BUG: fix Period.asfreq conversion near datetime(1, 1, 1) #19650

jbrockmendel · 2018-02-11T21:53:56Z

closes Period.asfreq issue caused(?) by C division rules #19643
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

jorisvandenbossche · 2018-02-11T22:27:46Z

Is there a reason you moved the tests?

jbrockmendel · 2018-02-11T23:16:20Z

Is there a reason you moved the tests?

Ongoing process of centralizing and de-duplicating tests. The moved tests cover code near the bug being fixed, so it seemed close enough to in-scope.

codecov · 2018-02-12T02:12:59Z

Codecov Report

Merging #19650 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #19650      +/-   ##
==========================================
+ Coverage   91.61%   91.61%   +<.01%     
==========================================
  Files         150      150              
  Lines       48887    48882       -5     
==========================================
- Hits        44786    44782       -4     
+ Misses       4101     4100       -1

Flag	Coverage Δ
#multiple	`89.98% <ø> (ø)`	⬆️
#single	`41.79% <ø> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/sparse/series.py	`95.25% <0%> (-0.02%)`	⬇️
pandas/core/sparse/frame.py	`94.81% <0%> (ø)`	⬆️
pandas/core/sparse/array.py	`91.46% <0%> (+0.08%)`	⬆️
pandas/core/ops.py	`96.86% <0%> (+0.12%)`	⬆️

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 740ad9a...e4e453b. Read the comment docs.

jorisvandenbossche · 2018-02-12T09:31:16Z

It feels you are rather spreading the Period tests to me? This are tests for the public Period class, and tests related to our scalar classes (Timestamp, Period, Timedelta, Interval) are centralized in the /tests/scalar/ directory. My preference is to keep it that way.
(I think the tslibs tests are currently tests on functionality contained in tslibs we want to test but is not public?)

jreback · 2018-02-12T11:20:32Z

lets create pandas/tests/scalar/period and move these tests to freq.py

jreback · 2018-02-12T11:17:15Z

pandas/_libs/tslibs/period.pyx

@@ -154,12 +154,30 @@ cdef inline int get_freq_group(int freq) nogil:
    return (freq // 1000) * 1000


-@cython.cdivision
+@cython.cdivision(False)  # specifically _dont_ use cdvision GH#19643


add a comment on why

rather does this actually matter now that you are always using floor division?

rather does this actually matter now that you are always using floor division?

Yes. With cdivision -1 // 7 returns 0. I find it really counter-intuitive, not at all surprising that this bug slipped through.

jreback · 2018-02-12T11:18:24Z

pandas/_libs/tslibs/period.pyx

+    Parameters
+    ----------
+    dinfo : date_info*
+    absdate : int64_t (as returned from get_python_ordinal or absdate_from_ymd)


not a great comment (nor does it follow doc-string conventions)

Will change.

jreback · 2018-02-12T11:18:31Z

pandas/_libs/tslibs/period.pyx

+
+    Parameters
+    ----------
+    dinfo : date_info*


That's what the asterisk is for (this notation is used in docstrings elsewhere). Have a preferred syntax?

ok, you are removing this anyhow IIRC? (in follow on)

jreback · 2018-02-12T11:18:59Z

pandas/_libs/tslibs/period.pyx

+    Parameters
+    ----------
+    dinfo : date_info*
+    abstime : double


can you be more specific here , e.g. what is abstime

The next line seconds elapsed since beginning of day described by absdate isn't clear?

jreback · 2018-02-12T11:19:16Z

pandas/_libs/tslibs/period.pyx

+
+    Returns
+    -------
+    code : int (always 0)


success / error

It's actually a dummy; there is no error-state. I guess just make it return None?

jbrockmendel · 2018-02-12T15:15:37Z

re test location: will change.

jreback · 2018-02-13T00:23:24Z

pandas/tests/tslibs/test_period_asfreq.py



 class TestPeriodFreqConversion(object):
+    @pytest.mark.parametrize('freq', ['A', 'Q', 'M', 'W', 'B', 'D'])
+    def test_asfreq_near_zero(self, freq):


~~can you add a whatsnew note for this~~
nvm saw that

jreback · 2018-02-13T00:23:31Z

pandas/tests/tslibs/test_period_asfreq.py

+        tup2 = (prev.year, prev.month, prev.day)
+        assert tup2 < tup1
+
+    @pytest.mark.xfail


can you add a reason

jreback · 2018-02-13T00:24:46Z

small comment. ping on green. ok with moving tests in another PR (to a period scalar sub-module) as discussed.

…elper14

jreback · 2018-02-13T11:35:18Z

The following asvs benchmarks (if any) failed.
[ 43.09%] ··· Running io.excel.Excel.time_read_excel                  1/3 failed
                   xlwt      failed

its possible this actually broke this asv

jbrockmendel · 2018-02-13T15:51:51Z

its possible this actually broke this asv

Is there a test this corresponds to?

jbrockmendel · 2018-02-13T15:55:20Z

It executes fine locally, will run asv on it to see if the timing is massively changed.

…elper14

jbrockmendel · 2018-02-13T16:34:29Z

asv runs fine on both 3.5 and 2.7

…elper14

jbrockmendel · 2018-02-15T04:18:10Z

Thoughts on this? I assumed the asv concern superseded the "ping on green"

jreback · 2018-02-16T18:17:23Z

pandas/_libs/tslibs/period.pyx

+
+    Parameters
+    ----------
+    dinfo : date_info*


ok, you are removing this anyhow IIRC? (in follow on)

jreback · 2018-02-16T18:18:01Z

pandas/_libs/tslibs/period.pyx

+    abstime : double
+        seconds elapsed since beginning of day described by absdate
+
+    Returns


this should no longer be needed. IOW this is the point of an errors declaration (and raising)

I agree, will be removing the leftover-from-C bits (and camelCase) in the next pass. For now this PR is focused on just the one bug.

jreback · 2018-02-16T18:18:17Z

pandas/_libs/tslibs/period.pyx

+    Parameters
+    ----------
+    dinfo : date_info*
+    abstime : double


jreback · 2018-02-16T18:18:39Z

pandas/_libs/tslibs/period.pyx

@@ -336,7 +407,7 @@ cdef int dInfoCalc_SetFromAbsTime(date_info *dinfo, double abstime) nogil:
    dinfo.hour = hour
    dinfo.minute = minute
    dinfo.second = second
-    return 0


why do you need a return?

I don't. Will remove when killing this function off.

just remove the return

jreback · 2018-02-18T16:26:20Z

pandas/_libs/tslibs/period.pyx

@@ -336,7 +407,7 @@ cdef int dInfoCalc_SetFromAbsTime(date_info *dinfo, double abstime) nogil:
    dinfo.hour = hour
    dinfo.minute = minute
    dinfo.second = second
-    return 0


just remove the return

jreback · 2018-02-18T16:26:29Z

pandas/_libs/tslibs/period.pyx

+    abstime : double
+        seconds elapsed since beginning of day described by absdate
+
+    Returns


we don't have Returns for None, remove

jreback · 2018-02-18T16:26:53Z

pandas/_libs/tslibs/period.pyx

+
+    Returns
+    -------
+    absdate : int days elapsed since datetime(1, 1, 1)


doc-string format, put the expl on the next line

jreback · 2018-02-18T16:27:02Z

pandas/_libs/tslibs/period.pyx

+
+    Returns
+    -------
+    qtr_freq : int describing the implied quarterly frequency


jreback · 2018-02-18T16:27:37Z

pandas/_libs/tslibs/period.pyx

+
+    Parameters
+    ----------
+    ordinal : int64_t


very odd that these take pointers

At this point these functions are translated as directly as possible from the C versions. In future passes I'd like to make them more pythonic too, will need to be careful about perf.

that’s fine

jreback · 2018-02-18T16:28:16Z

pandas/tests/tslibs/test_period_asfreq.py

@@ -1,10 +1,32 @@
 # -*- coding: utf-8 -*-

+import pytest
+
+from pandas.errors import OutOfBoundsDatetime
 from pandas._libs.tslibs.frequencies import get_freq


would not be averse to makeing this a submodule in future
tests/tslibls/period/test_asfreq

…elper14

jbrockmendel · 2018-02-19T02:39:50Z

I think all the requests have been hit.

jorisvandenbossche · 2018-02-19T07:58:49Z

Is there a reason you are adding the test in /tests/tslibs/test_period_asfreq.py and not in tests/scalar/test_period_asfreq.py ?

jbrockmendel · 2018-02-19T16:06:31Z

Is there a reason you are adding the test in /tests/tslibs/test_period_asfreq.py and not in tests/scalar/test_period_asfreq.py ?

test.tslibs is intended for testing tslibs in isolation. These asfreq tests are largely for period_helper (or were until recently) and so are self-contained. This is the right long-term home for these tests.

jorisvandenbossche · 2018-02-19T16:16:00Z

test.tslibs is intended for testing tslibs in isolation.

Yes, and the tests you added are testing behaviour of the Period class (not a private utility of tslib), which for the rest is tested in tests/scalar/test_period(_asfreq).py ?

jreback · 2018-02-19T16:19:49Z

@jorisvandenbossche I asked @jbrockmendel mendel to move these. not quite happy where the scalar tests are ATM, now whether actualy in tslib or the scalar/period is a matter of opinion here. I agree if these are actually user facing then they should be in the scalar/period/....

however as that doesn't exist yet (well in a minute it will), it was a bit confusing.

jorisvandenbossche · 2018-02-19T16:23:43Z

however as that doesn't exist yet (well in a minute it will)

tests/scalar/test_period_asfreq.py does actually already exist

agree if these are actually user facing then they should be in the scalar/period/....

This is the case here

We can reorganize tests, but let's do that in a separate PR, and add the tests here in the location where similar tests already are? (which is AFAIK tests/scalar/test_period_asfreq.py)

jreback · 2018-02-19T16:25:44Z

@jorisvandenbossche you will see shortly. the test reorg has been going on for quite a while and is moving in the right direction.

jreback · 2018-02-19T16:49:20Z

pandas/tests/tslibs/test_period_asfreq.py

+        tup1 = (per.year, per.hour, per.day)
+
+        prev = per - 1
+        assert (per - 1).ordinal == per.ordinal - 1


i think as a general rule if you are importing Period, then it should go in pandas/tests/scalar/period/* else it can go here (e.g. you are testing a helper function)

Seeing as how the recent addition of tests.scalar.period will necessitate a new look at this, I propose we merge this bug fix and the next pass (later today) can attempt to standardize this convention.

I think you can also simply move the tests there now (as travis has to run again anyhow since you just added a commit)

can you update for this comment

…elper14

jreback · 2018-02-20T11:41:06Z

lgtm. I changed the whatsnew slightly, so needs a rebase. ping on green. can fixup period tests after.

jbrockmendel · 2018-02-21T00:13:12Z

ping

jreback · 2018-02-21T00:24:13Z

pandas/tests/tslibs/test_period_asfreq.py

+        tup1 = (per.year, per.hour, per.day)
+
+        prev = per - 1
+        assert (per - 1).ordinal == per.ordinal - 1


can you update for this comment

…to phelper14

jorisvandenbossche · 2018-02-21T10:32:02Z

Thanks!

…19650)

jbrockmendel added 4 commits February 11, 2018 13:30

fix cdivision error, migrate period_asfreq tests

ed97203

docstrings, parametrize test

5faf231

fixup typo

5dbc03a

whatsnew

ffbd22f

lint fixup

785795c

gfyoung added Bug Period Period data type labels Feb 12, 2018

jreback requested changes Feb 12, 2018

View reviewed changes

jbrockmendel added 2 commits February 12, 2018 07:25

docstring edits

312c633

revert move of test_period_asfreq

dfe2542

jorisvandenbossche changed the title ~~Fix asfreq conversion~~ BUG: fix Period.asfreq conversion near datetime(1, 1, 1) Feb 12, 2018

jreback requested changes Feb 13, 2018

View reviewed changes

jreback added this to the 0.23.0 milestone Feb 13, 2018

jbrockmendel added 2 commits February 12, 2018 17:26

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

d749ecc

…elper14

reason for xfail

13295e2

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

a14bb59

…elper14

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

eafa0b0

…elper14

jbrockmendel mentioned this pull request Feb 16, 2018

Parametrize PeriodIndex tests #19659

Merged

4 tasks

jreback requested changes Feb 16, 2018

View reviewed changes

jreback removed this from the 0.23.0 milestone Feb 16, 2018

jbrockmendel mentioned this pull request Feb 17, 2018

Fix wraparound/overflow in date_range #19740

Closed

4 tasks

jreback requested changes Feb 18, 2018

View reviewed changes

jbrockmendel added 2 commits February 18, 2018 10:43

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

839e7f4

…elper14

docstring formatting, remove unused return values

96108cb

jreback reviewed Feb 19, 2018

View reviewed changes

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

0ad8623

…elper14

jreback added this to the 0.23.0 milestone Feb 20, 2018

jreback approved these changes Feb 20, 2018

View reviewed changes

Merge branch 'master' into phelper14

fcc92b0

jreback requested changes Feb 21, 2018

View reviewed changes

jbrockmendel added 2 commits February 20, 2018 17:33

move test

aefb3c7

Merge branch 'phelper14' of https://github.com/jbrockmendel/pandas in…

e4e453b

…to phelper14

jorisvandenbossche merged commit e5be6bd into pandas-dev:master Feb 21, 2018

jbrockmendel mentioned this pull request Feb 22, 2018

Continue porting period_helper; fix leftover asfreq bug #19834

Merged

jbrockmendel deleted the phelper14 branch February 22, 2018 05:45

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

BUG: fix Period.asfreq conversion near datetime(1, 1, 1) (pandas-dev#…

a60e325

…19650)

BUG: fix Period.asfreq conversion near datetime(1, 1, 1) #19650

BUG: fix Period.asfreq conversion near datetime(1, 1, 1) #19650

Conversation

jbrockmendel commented Feb 11, 2018 • edited Loading

jorisvandenbossche commented Feb 11, 2018

jbrockmendel commented Feb 11, 2018

codecov bot commented Feb 12, 2018 • edited Loading

Codecov Report

jorisvandenbossche commented Feb 12, 2018

jreback commented Feb 12, 2018 • 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

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

jbrockmendel commented Feb 12, 2018

jreback Feb 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Feb 13, 2018

jreback commented Feb 13, 2018

jbrockmendel commented Feb 13, 2018

jbrockmendel commented Feb 13, 2018

jbrockmendel commented Feb 13, 2018

jbrockmendel commented Feb 15, 2018

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

jbrockmendel commented Feb 19, 2018

jorisvandenbossche commented Feb 19, 2018

jbrockmendel commented Feb 19, 2018

jorisvandenbossche commented Feb 19, 2018

jreback commented Feb 19, 2018

jorisvandenbossche commented Feb 19, 2018

jreback commented Feb 19, 2018

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

jreback commented Feb 20, 2018

jbrockmendel commented Feb 21, 2018

Choose a reason for hiding this comment

jorisvandenbossche commented Feb 21, 2018

jbrockmendel commented Feb 11, 2018 •

edited

Loading

codecov bot commented Feb 12, 2018 •

edited

Loading

jreback commented Feb 12, 2018 •

edited

Loading

jreback Feb 13, 2018 •

edited

Loading