BUG/ENH: Handle NonexistentTimeError in date rounding #23406

mroeschke · 2018-10-29T07:13:37Z

closes Rounding valid timestamps near daylight savings jumps should not throw NonExistentTimeError #23324
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

Similar strategy as #22647, added a nonexistent keyword argument to round, ceil, and floor to control rounding when encountering a NonexistentTimeError

This is also fixes a bug in the nonexistent='shift' implementation in #22644 where dates with timezones with negative UTC offsets got shifted by an additional hour. This bug is naturally tested by these rounding tests

pep8speaks · 2018-10-29T07:13:41Z

Hello @mroeschke! Thanks for submitting the PR.

There are no PEP8 issues in the file pandas/core/indexes/datetimelike.py !
There are no PEP8 issues in the file pandas/tests/scalar/timestamp/test_unary_ops.py !
There are no PEP8 issues in the file pandas/tests/series/test_datetime_values.py !

codecov · 2018-10-29T08:00:58Z

Codecov Report

Merging #23406 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #23406      +/-   ##
==========================================
+ Coverage   92.18%   92.19%   +<.01%     
==========================================
  Files         161      161              
  Lines       51184    51192       +8     
==========================================
+ Hits        47185    47194       +9     
+ Misses       3999     3998       -1

Flag	Coverage Δ
#multiple	`90.62% <100%> (ø)`	⬆️
#single	`42.22% <55.55%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/indexes/datetimelike.py	`98.01% <100%> (ø)`	⬆️
pandas/util/_decorators.py	`91.34% <0%> (ø)`	⬆️
pandas/core/indexes/multi.py	`95.46% <0%> (ø)`	⬆️
pandas/util/testing.py	`86.84% <0%> (ø)`	⬆️
pandas/util/_test_decorators.py	`93.24% <0%> (ø)`	⬆️
pandas/util/_doctools.py	`12.87% <0%> (ø)`	⬆️
pandas/compat/pickle_compat.py	`75.6% <0%> (ø)`	⬆️
pandas/core/indexes/base.py	`96.62% <0%> (ø)`	⬆️
pandas/core/ops.py	`94.25% <0%> (+0.01%)`	⬆️
pandas/core/indexes/range.py	`95.79% <0%> (+0.02%)`	⬆️
... 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 a2e5994...b31f034. Read the comment docs.

gfyoung · 2018-10-29T13:35:41Z

pandas/tests/scalar/timestamp/test_unary_ops.py

+        result = getattr(ts, method)(freq, nonexistent='NaT')
+        assert result is NaT
+
+        with pytest.raises(pytz.NonExistentTimeError):


Anything meaningful in the error message? (question applies for all of your tests)

I think this just raises NonExistentTimeError: [time that was nonexistent] but I can test for the message as well.

gfyoung · 2018-10-29T23:53:08Z

doc/source/whatsnew/v0.24.0.txt

@@ -227,6 +227,7 @@ Other Enhancements
 - :class:`Series` and :class:`DataFrame` now support :class:`Iterable` in constructor (:issue:`2193`)
 - :class:`DatetimeIndex` gained :attr:`DatetimeIndex.timetz` attribute. Returns local time with timezone information. (:issue:`21358`)
 - :meth:`round`, :meth:`ceil`, and meth:`floor` for :class:`DatetimeIndex` and :class:`Timestamp` now support an ``ambiguous`` argument for handling datetimes that are rounded to ambiguous times (:issue:`18946`)
+- :meth:`round`, :meth:`ceil`, and meth:`floor` for :class:`DatetimeIndex` and :class:`Timestamp` now support an ``nonexistent`` argument for handling datetimes that are rounded to nonexistent times (:issue:`22647`)


an nonexistent --> a nonexistent

Do you think this requires a mini-section to explain behavior?

I can link the section in the timeseries.rst that explains nonexistent behavior.

Awesome. That works just as well.

gfyoung · 2018-10-30T00:00:45Z

pandas/tests/series/test_datetime_values.py

+
+        with pytest.raises(pytz.NonExistentTimeError,
+                           message='2018-03-11 02:00:00'):
+            getattr(s.dt, method)(freq, nonexistent='raise')


I'm almost inclined to have parameterization on nonexistent, but up to you.

I'm inclined to keep the format of this test. IMO parameterization over nonexistent would obfuscate the test too much.

No problem. Just a thought. 👍

More general question, to what extent are we using pytest.raises vs tm.assert_raises_regex ?

cc @jreback

ok with pytest.raises to the extent we are just checking the type of the error. so ok here

@jreback : pytest.raises is also checking the error message here...

(that's why I asked about this)

jreback · 2018-10-30T12:16:05Z

pandas/_libs/tslibs/conversion.pyx

@@ -852,6 +851,8 @@ def tz_localize_to_utc(ndarray[int64_t] vals, object tz, object ambiguous=None,
        Py_ssize_t i, idx, pos, ntrans, n = len(vals)
        int64_t *tdata
        int64_t v, left, right, val, v_left, v_right
+        int64_t remaining_minutes, new_local


can you add with the previous line here

jreback · 2018-10-30T12:16:40Z

pandas/_libs/tslibs/conversion.pyx

@@ -852,6 +851,8 @@ def tz_localize_to_utc(ndarray[int64_t] vals, object tz, object ambiguous=None,
        Py_ssize_t i, idx, pos, ntrans, n = len(vals)
        int64_t *tdata
        int64_t v, left, right, val, v_left, v_right
+        int64_t remaining_minutes, new_local
+        int delta_idx_offset, delta_idx
        ndarray[int64_t] result, result_a, result_b, dst_hours


don't we use Py_ssize_t for indexers?

jreback · 2018-10-30T12:17:34Z

pandas/_libs/tslibs/nattype.pyx

@@ -484,6 +484,17 @@ class NaTType(_NaT):
            - 'raise' will raise an AmbiguousTimeError for an ambiguous time

            .. versionadded:: 0.24.0
+        nonexistent : 'shift', 'NaT', default 'raise'


if any way to share these doc-strings would be great, maybe templating?

@jbrockmendel do you happen to know why for some of these NaT methods we aren't just passing is Timestamp.[method].__doc__ instead of copying these docstrings?

Because of the dependency structure. We import NaT in timestznps but not the other way around.

jreback · 2018-10-30T12:18:04Z

pandas/_libs/tslibs/timestamps.pyx

+              existing time
+            - 'NaT' will return NaT where there are nonexistent times
+            - 'raise' will raise an NonExistentTimeError if there are
+              nonexistent times


same comment as above about the doc-strings

mroeschke · 2018-11-01T06:31:45Z

Addressed your comments @jreback. I can make another issue between docstring templating between Timestamp and NaT as many methods share the same docstring (not just tz_localize)

jreback · 2018-11-02T14:15:34Z

thanks @mroeschke

yes would like to template the doc-strings to avoid duplication.

…xamples * repo_org/master: (66 commits) CLN: doc string (pandas-dev#23469) DOC: Add cookbook entry for triangular correlation matrix (GH22840) (pandas-dev#23032) add number of Errors, Warnings to scripts/validate_docstrings.py (pandas-dev#23150) BUG: Allow freq conversion from dt64 to period (pandas-dev#23460) ENH: Add FrozenList.union and .difference (pandas-dev#23394) REF: cython cleanup, typing, optimizations (pandas-dev#23464) strictness and checks for Timedelta _simple_new (pandas-dev#23433) Fixing flake8 problems new to flake8 3.6.0 (pandas-dev#23472) DOC: Updating the docstring of Series.dot (pandas-dev#22890) TST: Fixturize series/test_analytics.py (pandas-dev#22755) BUG/ENH: Handle NonexistentTimeError in date rounding (pandas-dev#23406) PERF: speed up concat on Series by making _get_axis_number() a classmethod (pandas-dev#23404) REF: Remove DatetimelikeArrayMixin._shallow_copy (pandas-dev#23430) REF: strictness/simplification in DatetimeArray/Index _simple_new (pandas-dev#23431) REF: cython cleanup, typing, optimizations (pandas-dev#23456) TST: tweak Hypothesis configuration and idioms (pandas-dev#23441) BUG: fix HDFStore.append with all empty strings error (GH12242) (pandas-dev#23435) TST: Skip 32bit failing IntervalTree tests (pandas-dev#23442) BUG: Deprecate nthreads argument (pandas-dev#23112) style: fix import format at pandas/core/reshape (pandas-dev#23387) ...

Matt Roeschke added 2 commits October 28, 2018 21:26

nonexistent rounding

21cbe4f

Fix shifting bug and add tests and whatsnew

35711b0

gfyoung added Bug Datetime Datetime data dtype Timezones Timezone data dtype labels Oct 29, 2018

gfyoung reviewed Oct 29, 2018

View reviewed changes

mroeschke added this to the 0.24.0 milestone Oct 29, 2018

Matt Roeschke added 2 commits October 29, 2018 15:57

Merge remote-tracking branch 'upstream/master' into nonexistent_round

eb19fff

Add expected nonexistent time with raise

580d231

gfyoung reviewed Oct 29, 2018

View reviewed changes

fix typo and add reference

621f921

gfyoung reviewed Oct 30, 2018

View reviewed changes

jreback requested changes Oct 30, 2018

View reviewed changes

Matt Roeschke added 2 commits October 30, 2018 17:55

change to py_ssize_t

a60b689

Combine in64_t cdef

b31f034

jreback approved these changes Nov 2, 2018

View reviewed changes

jreback merged commit 2073fc2 into pandas-dev:master Nov 2, 2018

mroeschke deleted the nonexistent_round branch November 2, 2018 15:18

tm9k1 pushed a commit to tm9k1/pandas that referenced this pull request Nov 19, 2018

BUG/ENH: Handle NonexistentTimeError in date rounding (pandas-dev#23406)

168f36c

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

BUG/ENH: Handle NonexistentTimeError in date rounding (pandas-dev#23406)

19e7bce

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

BUG/ENH: Handle NonexistentTimeError in date rounding (pandas-dev#23406)

cd17115

quangngd mentioned this pull request Nov 17, 2023

BUG: Bad shift_forward around UTC+0 when DST #56017

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG/ENH: Handle NonexistentTimeError in date rounding #23406

BUG/ENH: Handle NonexistentTimeError in date rounding #23406

mroeschke commented Oct 29, 2018

pep8speaks commented Oct 29, 2018

codecov bot commented Oct 29, 2018 •

edited

Loading

gfyoung Oct 29, 2018 •

edited

Loading

mroeschke Oct 29, 2018

gfyoung Oct 29, 2018

gfyoung Oct 29, 2018

mroeschke Oct 29, 2018

gfyoung Oct 29, 2018

gfyoung Oct 30, 2018 •

edited

Loading

mroeschke Oct 30, 2018

gfyoung Oct 30, 2018

jreback Oct 30, 2018

gfyoung Oct 30, 2018 •

edited

Loading

jreback Oct 30, 2018

jreback Oct 30, 2018

jreback Oct 30, 2018

mroeschke Oct 31, 2018

jbrockmendel Nov 1, 2018

jreback Oct 30, 2018

mroeschke commented Nov 1, 2018

jreback commented Nov 2, 2018

BUG/ENH: Handle NonexistentTimeError in date rounding #23406

BUG/ENH: Handle NonexistentTimeError in date rounding #23406

Conversation

mroeschke commented Oct 29, 2018

pep8speaks commented Oct 29, 2018

codecov bot commented Oct 29, 2018 • edited Loading

Codecov Report

gfyoung Oct 29, 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

gfyoung Oct 30, 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

gfyoung Oct 30, 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

mroeschke commented Nov 1, 2018

jreback commented Nov 2, 2018

codecov bot commented Oct 29, 2018 •

edited

Loading

gfyoung Oct 29, 2018 •

edited

Loading

gfyoung Oct 30, 2018 •

edited

Loading

gfyoung Oct 30, 2018 •

edited

Loading