TST: Added fromisocalendar test cases #30395

ShaharNaveh · 2019-12-21T21:34:17Z

closes COMPAT: Implement Timestamp.fromisocalendar #28115
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

ShaharNaveh · 2019-12-21T21:34:28Z

pandas/_libs/tslibs/timestamps.pyx

jbrockmendel · 2019-12-21T23:03:57Z

This is a good start. Next up you need to add tests for the new functionality. Those will go in pandas.tests.scalar.timestamp.test_timestamp.TestTimestampConstructors. Look at the tests we have there for the other similar Timestamp classmethods to get an idea of what the tests should look like

jbrockmendel · 2019-12-21T23:05:22Z

if you look at the CI results you'll see some red checkmarks. you're going to need to click through to find out why this is failing so you can fix it (spoiler: test_missing_public_nat_methods)

jbrockmendel · 2019-12-22T04:12:49Z

pandas/_libs/tslibs/nattype.pyx

+        Return a new Timestamp corresponding to the
+        ISO calendar date specified by year, week and day.
+        """
+    )


good, you figured out the test that was failiing before

jbrockmendel · 2019-12-22T04:18:37Z

Good going so far. You changed it to raise in older python versions and fixed the test that was broken before. But now if you look at the CI you'll see there is a different test that is broken.

So the next step is to look at the CI log to tell what test(s) are failing (spoiler: the test you added is failing when run in older pythons) and figure out how to fix it. (Ideally also you'll run the tests locally to make sure they pass on your computer before pushing, let me know if you have trouble with that)

This becomes more intuitive with experience.

ShaharNaveh · 2019-12-22T04:26:23Z

Ideally also you'll run the tests locally to make sure they pass on your computer before pushing

I did, the problem is I have python 3.8

Anyway, I'll fix it.

Is there's more elegant way rather than making an if statement checking the python version?

pandas/tests/scalar/timestamp/test_timestamp.py

jbrockmendel · 2019-12-22T16:36:24Z

Good progress

pandas/_libs/tslibs/timestamps.pyx

jreback · 2019-12-23T17:33:43Z

pandas/_libs/tslibs/timestamps.pyx

+    def fromisocalendar(cls, year, week, day):
+        """
+        Timestamp.fromisocalendar(year, week, day)
+


can you add a full doc-string here

jreback · 2019-12-23T17:34:06Z

pandas/_libs/tslibs/timestamps.pyx

+        Return a new Timestamp corresponding to the
+        ISO calendar date specified by year, week and day.
+        """
+        import pandas.compat as compat


@jbrockmendel can we not import this at the top? (else we should have these constants in a .pyx somewhere

importing this at the top is not an option, but we can either put it in e.g. tslibs.util or from cpython.version cimport PY_MINOR_VERSION.

right should prob make a standard way of doing this (though i don't think we need to have version checks too often in cython these days)

pandas/tests/scalar/timestamp/test_timestamp.py

jbrockmendel · 2019-12-23T18:03:10Z

So I just tried pd.Timestamp.fromisocalendar on 0.25.3 and it works fine (i.e. correctly returns a Timestamp). I think we need a test for this, but the actual implementation in the cython file is unnecessary.

ShaharNaveh · 2019-12-24T16:07:59Z

So I just tried pd.Timestamp.fromisocalendar on 0.25.3 and it works fine (i.e. correctly returns a Timestamp). I think we need a test for this, but the actual implementation in the cython file is unnecessary.

So remove the code here?

https://github.com/MomIsBestFriend/pandas/blob/88468fa3604457319401b126764ed49be3e6d15f/pandas/_libs/tslibs/timestamps.pyx#L340

(Removed the new created method?)

jbrockmendel · 2019-12-25T00:24:20Z

(Removed the new created method?)

I think the test will still pass, so yes.

ShaharNaveh · 2019-12-25T00:41:47Z

Since I really didn't add anything new to the code base (except test cases).

Can #28115 be closed?

TomAugspurger · 2019-12-30T16:04:24Z

@MomIsBestFriend seems like there are some CI failures for py37 and earlier.

TomAugspurger · 2019-12-31T13:07:23Z

pandas/_libs/tslibs/nattype.pyx

@@ -426,6 +426,7 @@ class NaTType(_NaT):
    toordinal = _make_error_func('toordinal', datetime)
    tzname = _make_error_func('tzname', datetime)
    utcoffset = _make_error_func('utcoffset', datetime)
+    fromisocalendar = _make_error_func('fromisocalendar', datetime)


Does this need to be an in if PY38 block? In other words, will there be a Timestamp.fromisocalendar for pandas 1.0 with python 3.7, or just py38 and above?

pandas/_libs/tslibs/nattype.pyx

jreback · 2020-01-01T17:59:47Z

pandas/tests/scalar/test_nat.py

@@ -140,6 +141,10 @@ def test_round_nat(klass, method, freq):
    ],
 )
 def test_nat_methods_raise(method):


can you put a skip on the argument instead, use pytest.param

Didn't even knew that was an option. thank you for being specific!

jreback · 2020-01-03T01:41:54Z

code & tests lgtm. can you add a whatsnew node; other enhancements section is ok, just say added compatiblity with datetime.isocalender for PY >= 38

jbrockmendel · 2020-01-03T01:45:52Z

pandas/_libs/tslibs/nattype.pyx

@@ -19,6 +19,8 @@ from pandas._libs.tslibs.util cimport (
    get_nat, is_integer_object, is_float_object, is_datetime64_object,
    is_timedelta64_object)

+import pandas.compat as compat


we really dont want this import here. how about from cpython.version cimport PY_MINOR_VERSION

Fixed.

And just for I know for the next time, why we really don't want this import?

we want to keep the dependency structure simple, and _libs, particularly tslibs is at the base of that structure, so avoids imports from elsewhere in pandas

jreback · 2020-01-06T13:30:44Z

thanks @MomIsBestFriend

ShaharNaveh changed the title ~~Added class method~~ COMPACT: fromisocalendar method to Timestamp Dec 21, 2019

jbrockmendel reviewed Dec 21, 2019

View reviewed changes

pandas/_libs/tslibs/timestamps.pyx Outdated Show resolved Hide resolved

ShaharNaveh force-pushed the Fix-28115 branch 2 times, most recently from e1a6106 to 82065c5 Compare December 22, 2019 01:22

ShaharNaveh changed the title ~~COMPACT: fromisocalendar method to Timestamp~~ COMPAT: fromisocalendar method to Timestamp Dec 22, 2019

jbrockmendel reviewed Dec 22, 2019

View reviewed changes

ShaharNaveh marked this pull request as ready for review December 22, 2019 05:16

jbrockmendel reviewed Dec 22, 2019

View reviewed changes

pandas/tests/scalar/timestamp/test_timestamp.py Show resolved Hide resolved

jbrockmendel reviewed Dec 22, 2019

View reviewed changes

pandas/tests/scalar/timestamp/test_timestamp.py Outdated Show resolved Hide resolved

jbrockmendel reviewed Dec 22, 2019

View reviewed changes

pandas/tests/scalar/timestamp/test_timestamp.py Outdated Show resolved Hide resolved

mroeschke reviewed Dec 22, 2019

View reviewed changes

pandas/_libs/tslibs/timestamps.pyx Outdated Show resolved Hide resolved

ShaharNaveh force-pushed the Fix-28115 branch 2 times, most recently from d742180 to 280d04e Compare December 22, 2019 20:35

gfyoung added Compat pandas objects compatability with Numpy or Python functions Datetime Datetime data dtype labels Dec 22, 2019

jreback requested changes Dec 23, 2019

View reviewed changes

jbrockmendel reviewed Dec 23, 2019

View reviewed changes

pandas/tests/scalar/timestamp/test_timestamp.py Show resolved Hide resolved

ShaharNaveh force-pushed the Fix-28115 branch from 280d04e to 88468fa Compare December 24, 2019 16:01

ShaharNaveh force-pushed the Fix-28115 branch from 88468fa to 029954b Compare December 25, 2019 00:36

ShaharNaveh changed the title ~~COMPAT: fromisocalendar method to Timestamp~~ TST: Added fromisocalendar test cases Dec 25, 2019

ShaharNaveh force-pushed the Fix-28115 branch 2 times, most recently from 01b9647 to e7327e7 Compare December 29, 2019 11:36

ShaharNaveh force-pushed the Fix-28115 branch from e7327e7 to de89a09 Compare December 31, 2019 12:16

TomAugspurger reviewed Dec 31, 2019

View reviewed changes

ShaharNaveh force-pushed the Fix-28115 branch 2 times, most recently from d6553e5 to 5315efc Compare December 31, 2019 13:14

MomIsBestFriend added 3 commits January 1, 2020 14:56

COMPAT: Added 'Timestamp.fromisocalendar'

cbea9c3

Added full docstring

3f37a97

Fixed tests

e4696b1

ShaharNaveh force-pushed the Fix-28115 branch 2 times, most recently from d4956e1 to e322603 Compare January 1, 2020 13:55

Put fromisocalendar in PY38 block

e322603

jreback removed this from the 1.0 milestone Jan 1, 2020

jreback requested changes Jan 1, 2020

View reviewed changes

skip 'fromisocalendar' method if not in python 3.8 and above

ad9c411

ShaharNaveh requested a review from jreback January 1, 2020 19:44

TomAugspurger approved these changes Jan 2, 2020

View reviewed changes

TomAugspurger added this to the 1.0 milestone Jan 2, 2020

jbrockmendel reviewed Jan 3, 2020

View reviewed changes

MomIsBestFriend added 4 commits January 5, 2020 09:14

Merge remote-tracking branch 'upstream/master' into Fix-28115

deaaa57

Replaced compat.PY38 with cpython.version.PY_MINOR_VERSION

a06dbca

Added what's new note

c8ad230

Merge remote-tracking branch 'upstream/master' into Fix-28115

9f800d6

jreback approved these changes Jan 6, 2020

View reviewed changes

jreback merged commit 0bbfa0a into pandas-dev:master Jan 6, 2020

ShaharNaveh deleted the Fix-28115 branch January 6, 2020 13:37

ShaharNaveh mentioned this pull request Feb 14, 2020

Avoid importing from pandas at _libs files #31977

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TST: Added fromisocalendar test cases #30395

TST: Added fromisocalendar test cases #30395

ShaharNaveh commented Dec 21, 2019 •

edited

Loading

ShaharNaveh commented Dec 21, 2019

jbrockmendel commented Dec 21, 2019

jbrockmendel commented Dec 21, 2019

jbrockmendel Dec 22, 2019

jbrockmendel commented Dec 22, 2019

ShaharNaveh commented Dec 22, 2019 •

edited

Loading

jbrockmendel commented Dec 22, 2019

jreback Dec 23, 2019

jreback Dec 23, 2019

jbrockmendel Dec 23, 2019

jreback Dec 23, 2019

jbrockmendel commented Dec 23, 2019

ShaharNaveh commented Dec 24, 2019 •

edited

Loading

jbrockmendel commented Dec 25, 2019

ShaharNaveh commented Dec 25, 2019

TomAugspurger commented Dec 30, 2019

TomAugspurger Dec 31, 2019

jreback Jan 1, 2020

ShaharNaveh Jan 1, 2020

jreback commented Jan 3, 2020

jbrockmendel Jan 3, 2020

ShaharNaveh Jan 5, 2020

jbrockmendel Jan 5, 2020

jreback commented Jan 6, 2020

TST: Added fromisocalendar test cases #30395

TST: Added fromisocalendar test cases #30395

Conversation

ShaharNaveh commented Dec 21, 2019 • edited Loading

ShaharNaveh commented Dec 21, 2019

jbrockmendel commented Dec 21, 2019

jbrockmendel commented Dec 21, 2019

Choose a reason for hiding this comment

jbrockmendel commented Dec 22, 2019

ShaharNaveh commented Dec 22, 2019 • edited Loading

jbrockmendel commented Dec 22, 2019

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 Dec 23, 2019

ShaharNaveh commented Dec 24, 2019 • edited Loading

jbrockmendel commented Dec 25, 2019

ShaharNaveh commented Dec 25, 2019

TomAugspurger commented Dec 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Jan 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Jan 6, 2020

ShaharNaveh commented Dec 21, 2019 •

edited

Loading

ShaharNaveh commented Dec 22, 2019 •

edited

Loading

ShaharNaveh commented Dec 24, 2019 •

edited

Loading