Checks for tz/tzinfo validity in Timestamp.new #17943

jbrockmendel · 2017-10-22T22:50:58Z

closes ERR: Timestamp.__new__ accepts both tz and tzinfo #17690
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

closes pandas-dev#17690 possibly pandas-dev#5168

codecov · 2017-10-23T05:49:55Z

Codecov Report

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

@@            Coverage Diff             @@
##           master   #17943      +/-   ##
==========================================
- Coverage   91.24%   91.23%   -0.01%     
==========================================
  Files         163      163              
  Lines       50173    50173              
==========================================
- Hits        45778    45774       -4     
- Misses       4395     4399       +4

Flag	Coverage Δ
#multiple	`89.04% <ø> (ø)`	⬆️
#single	`40.29% <ø> (-0.06%)`	⬇️

Impacted Files	Coverage Δ
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.75% <0%> (-0.1%)`	⬇️
pandas/io/msgpack/_version.py	`44.65% <0%> (+1.9%)`	⬆️

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 8137209...98b5b17. Read the comment docs.

jreback

needs a whatsnew note.

jreback · 2017-10-23T10:25:26Z

pandas/_libs/tslib.pyx

+            elif tz is not None:
+                raise ValueError('Can provide at most one of tz, tzinfo')
+            elif ts_input is not _no_input:
+                raise ValueError('When creating a Timestamp with this type '


misspell

what case is this trying to catch? this is a confusing error message

Good catch on the spell-check, thanks.

Under the status quo, the tzinfo kwarg is used in only one branch:

if ts_input is _no_input: # User passed keyword arguments. return Timestamp(datetime(year, month, day, hour or 0, minute or 0, second or 0, microsecond or 0, tzinfo), tz=tzinfo)

In all other cases tzinfo is silently ignored. This raises instead of ignoring the input. As you noted below, it may be better to change this behavior.

You're right the message is oddly worded. "this type of inputs" in this case is "anything other than the keyword arguments for datetime(...)". Suggestions for wording? (unnecessary if we change the behavior instead)

jreback · 2017-10-23T10:26:42Z

pandas/tests/scalar/test_timestamp.py

+
+        with tm.assert_raises_regex(ValueError, 'Use `tz` instead'):
+            dt = datetime(2017, 10, 22)
+            Timestamp(dt, tzinfo=utc)


this is a valid case. you can just do (internally).

tz, tzinfo = tzinfo, None

Hmm this gets really verbose since we need to check in a handful of different places. I guess get correctness right first, then circle back for brevity.

jreback · 2017-10-23T10:27:32Z

pandas/tests/scalar/test_timestamp.py

+            Timestamp('2017-10-22', tzinfo=utc, tz='UTC')
+
+        with tm.assert_raises_regex(ValueError, 'Use `tzinfo` instead'):
+            Timestamp(year=2017, month=10, day=22, tz='UTC')


this is also valid, you just can't pass tzinfo thru to the datetime constructor. rather you need to construct the Timestamp, then localize.

OK. So the block that currently reads:

if ts_input is _no_input: # User passed keyword arguments. return Timestamp(datetime(year, month, day, hour or 0, minute or 0, second or 0, microsecond or 0, tzinfo), tz=tzinfo)

should become something like

if ts_input is _no_input: # User passed keyword arguments. if tz is None: tz = tzinfo return Timestamp(datetime(year, month, day, hour or 0, minute or 0, second or 0, microsecond or 0, tzinfo), tz=tz)

?

jreback · 2017-10-23T19:13:33Z

pandas/_libs/tslib.pyx

+            if tz is None:
+                # This allows us to handle the case where the user passes
+                # `tz` and not `tzinfo`.
+                tz = tzinfo
            return Timestamp(datetime(year, month, day, hour or 0,


you should set tzinfo to be None here

jreback · 2017-10-23T19:13:44Z

pandas/_libs/tslib.pyx

@@ -387,6 +400,10 @@ class Timestamp(_Timestamp):
                                      year or 0, month or 0, day or 0,
                                      hour), tz=hour)

+        if tzinfo is not None:


this shouldn't be necessary if you do as the above.

Why? Changing line 390 above doesn't affect anything at this point in the func.

you have the exact same check above (if tzinfo is not none), so you shoul do this up there. (line 377)

OK. That requires conditioning on elif not is_integer_object(freq). Just pushed.

jreback · 2017-10-23T19:14:42Z

doc/source/whatsnew/v0.21.0.txt

@@ -830,6 +830,7 @@ Other API Changes
 - Restricted DateOffset keyword arguments.  Previously, ``DateOffset`` subclasses allowed arbitrary keyword arguments which could lead to unexpected behavior.  Now, only valid arguments will be accepted. (:issue:`17176`).
 - Pandas no longer registers matplotlib converters on import. The converters
  will be registered and used when the first plot is draw (:issue:`17710`)
+- :class:`Timestamp` will no longer silently ignore unused `tz` or `tzinfo` arguments (:issue:`17690`)


or invalid tzinfo

jbrockmendel · 2017-10-24T15:43:56Z

Just pushed fixes for test errors. In a nutshell: the more verbose version is also more robust.

jbrockmendel · 2017-10-25T00:16:39Z

Travis error in TestClipboard.test_round_trip_valid_encodings looks unrelated.

…mestamp-init

jreback · 2017-10-27T10:30:40Z

doc/source/whatsnew/v0.21.0.txt

@@ -830,6 +830,7 @@ Other API Changes
 - Restricted DateOffset keyword arguments.  Previously, ``DateOffset`` subclasses allowed arbitrary keyword arguments which could lead to unexpected behavior.  Now, only valid arguments will be accepted. (:issue:`17176`).
 - Pandas no longer registers matplotlib converters on import. The converters
  will be registered and used when the first plot is draw (:issue:`17710`)
+- :class:`Timestamp` will no longer silently ignore unused or invalid `tz` or `tzinfo` arguments (:issue:`17690`)


can you move to 0.22.0 same section

…mestamp-init

jreback · 2017-10-27T20:30:52Z

thanks!

jbrockmendel added 2 commits October 22, 2017 15:49

Checks for tz/tzinfo validity in Timestamp.__new__

7914f55

closes pandas-dev#17690 possibly pandas-dev#5168

Fix exception type in tests

5c89630

jreback requested changes Oct 23, 2017

View reviewed changes

jreback added Error Reporting Incorrect or improved errors from pandas Timezones Timezone data dtype labels Oct 23, 2017

jbrockmendel added 2 commits October 23, 2017 08:34

typo fixup; whatsnew note

6a2f766

handle tz/tzinfo in cases where they dont disagree

b4c98fb

jreback requested changes Oct 23, 2017

View reviewed changes

jreback reviewed Oct 23, 2017

View reviewed changes

jbrockmendel added 2 commits October 23, 2017 16:47

Edits per reviewer requests

8f304a6

revert to more verbose but more correct version

f812e90

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

d90bc25

…mestamp-init

jreback requested changes Oct 27, 2017

View reviewed changes

jbrockmendel added 2 commits October 27, 2017 09:07

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

4839ffb

…mestamp-init

move whatsnew note to 0.22.0

98b5b17

jreback added this to the 0.22.0 milestone Oct 27, 2017

jreback approved these changes Oct 27, 2017

View reviewed changes

jreback merged commit f7f33a7 into pandas-dev:master Oct 27, 2017

jbrockmendel deleted the timestamp-init branch October 27, 2017 22:13

jbrockmendel mentioned this pull request Oct 29, 2017

ENH/DOC: call for Timestamp #5168

Closed

jbrockmendel added a commit to jbrockmendel/pandas that referenced this pull request Oct 30, 2017

follow-up to pandas-dev#17943, closes pandas-dev#5168

d99b95a

jreback pushed a commit that referenced this pull request Oct 31, 2017

follow-up to #17943, closes #5168 (#18033)

4152a31

peterpanmj pushed a commit to peterpanmj/pandas that referenced this pull request Oct 31, 2017

Checks for tz/tzinfo validity in Timestamp.__new__ (pandas-dev#17943)

0f9fc73

peterpanmj pushed a commit to peterpanmj/pandas that referenced this pull request Oct 31, 2017

follow-up to pandas-dev#17943, closes pandas-dev#5168 (pandas-dev#18033)

b83f490

No-Stream pushed a commit to No-Stream/pandas that referenced this pull request Nov 28, 2017

Checks for tz/tzinfo validity in Timestamp.__new__ (pandas-dev#17943)

05c77a5

No-Stream pushed a commit to No-Stream/pandas that referenced this pull request Nov 28, 2017

follow-up to pandas-dev#17943, closes pandas-dev#5168 (pandas-dev#18033)

39dc6f4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Checks for tz/tzinfo validity in Timestamp.new #17943

Checks for tz/tzinfo validity in Timestamp.new #17943

jbrockmendel commented Oct 22, 2017

codecov bot commented Oct 23, 2017 •

edited

Loading

jreback left a comment

jreback Oct 23, 2017

jbrockmendel Oct 23, 2017

jreback Oct 23, 2017

jbrockmendel Oct 23, 2017

jreback Oct 23, 2017

jbrockmendel Oct 23, 2017

jreback Oct 23, 2017

jbrockmendel Oct 23, 2017

jreback Oct 23, 2017

jbrockmendel Oct 23, 2017

jreback Oct 23, 2017

jbrockmendel Oct 23, 2017

jreback Oct 23, 2017

jbrockmendel commented Oct 24, 2017

jbrockmendel commented Oct 25, 2017

jreback Oct 27, 2017

jreback commented Oct 27, 2017

Checks for tz/tzinfo validity in Timestamp.__new__ #17943

Checks for tz/tzinfo validity in Timestamp.__new__ #17943

Conversation

jbrockmendel commented Oct 22, 2017

codecov bot commented Oct 23, 2017 • edited Loading

Codecov Report

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

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 Oct 24, 2017

jbrockmendel commented Oct 25, 2017

Choose a reason for hiding this comment

jreback commented Oct 27, 2017

Checks for tz/tzinfo validity in Timestamp.new #17943

Checks for tz/tzinfo validity in Timestamp.new #17943

codecov bot commented Oct 23, 2017 •

edited

Loading