Fix Timestamp.round errors #22802

miccoli · 2018-09-21T20:11:57Z

closes Inconsistent behaviour in Timestamp.round #22591
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry
rebase for the new ambiguous kwarg, resolve conflicts.

pep8speaks · 2018-09-21T20:12:01Z

Hello @miccoli! 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/indexes/datetimes/test_scalar_compat.py !
There are no PEP8 issues in the file pandas/tests/scalar/timestamp/test_unary_ops.py !

codecov · 2018-09-21T22:40:59Z

Codecov Report

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

@@            Coverage Diff             @@
##           master   #22802      +/-   ##
==========================================
+ Coverage   92.18%   92.19%   +<.01%     
==========================================
  Files         169      169              
  Lines       50821    50827       +6     
==========================================
+ Hits        46851    46860       +9     
+ Misses       3970     3967       -3

Flag	Coverage Δ
#multiple	`90.61% <100%> (ø)`	⬆️
#single	`42.37% <33.33%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/indexes/datetimelike.py	`98.1% <100%> (ø)`	⬆️
pandas/core/strings.py	`98.63% <0%> (ø)`	⬆️
pandas/core/indexes/multi.py	`95.45% <0%> (ø)`	⬆️
pandas/core/frame.py	`97.2% <0%> (ø)`	⬆️
pandas/core/generic.py	`96.67% <0%> (ø)`	⬆️
pandas/io/formats/style.py	`96.43% <0%> (ø)`	⬆️
pandas/core/series.py	`93.74% <0%> (ø)`	⬆️
pandas/core/nanops.py	`95.14% <0%> (+0.01%)`	⬆️
pandas/io/parsers.py	`95.6% <0%> (+0.05%)`	⬆️
pandas/core/internals/managers.py	`96.66% <0%> (+0.1%)`	⬆️
... 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 302d43a...988e5e3. Read the comment docs.

jbrockmendel · 2018-09-21T23:20:04Z

pandas/_libs/tslibs/timestamps.pyx

+    return _floor_int64(v + u//2, u)
+
+
+def round_nsint64(values, mode: RoundTo, freq):


I don’t think cython recognizes RoundTwo as a type unless it is a cdef class. Not entirely sure on that.

Yes, I meant this more as a comment than an actual directive. Fixed in 4490269

pandas/_libs/tslibs/timestamps.pyx

jbrockmendel · 2018-09-21T23:21:06Z

pandas/_libs/tslibs/timestamps.pyx

+        d[mask] += 1
+        return d * unit
+
+    raise NotImplementedError(mode)


Is this reachable?

This code is unreachable and the NotImplementedError was cruft from a previous implementation.

In 4490269 I inserted an assert False to avoid the code returning None if a bug is introduced by changing the RoundTo class.

pandas/pandas/_libs/tslibs/timestamps.pyx

Lines 134 to 135 in 5232948

# this should be unreachable

assert False, "should never arrive here"

mroeschke

Looks fairly good:

Nit: Could you rename the single variable names to something a little more descriptive (e.g. d to divisor, etc)
Could you add similar DatetimeIndex tests as well?

jreback

can you add a whatsnew note for 0.24.0

pandas/_libs/tslibs/timestamps.pyx

jreback · 2018-09-23T11:59:33Z

pandas/tests/scalar/timestamp/test_unary_ops.py

+        unit = to_offset(freq).nanos
+        # test floor
+        result = dt.floor(freq)
+        assert result.value % unit == 0, "floor not a %s multiple" % (freq, )


can you add a blank line between cases

jreback · 2018-09-23T12:01:12Z

pandas/tests/scalar/timestamp/test_unary_ops.py

+        assert result.value % unit == 0, "floor not a %s multiple" % (freq, )
+        assert 0 <= dt.value - result.value < unit, "floor error"
+        # test ceil
+        result = dt.ceil(freq)


are there round errors in timedelta ops as well?

Yes, also timedeltas are affected by rounding errors, but it is very unlikely that these will be found in real world applications.

BTW I found #22591 processing real scientific data, in which I had timestamps for 2018 measurements with about 1 µs resolution. On the contrary I cannot imagine of an application in which you have 50 years time deltas with such a small resolution.

Of course you could create a round-trip example:

Timedate -> Timedelta -> round() -> Timedate

which gives different results with respect to

Timedate -> round().

Nevertheless I would suggest to postpone the resolution of the timedelta issue after this PR is merged.

(Please note also that the current complex machinery, which tries to improve rounding errors, is implemented only for Timedate and not for Timedelta.)

no i get it, that's why I asked. Ok to fix later, esp if its not very common.

jreback · 2018-09-23T13:26:29Z

just merged #22647, @miccoli will need to rebase to accomadate the ambiguous kwarg

miccoli · 2018-09-23T15:01:26Z

@mroeschke : I think I addressed your requests
1. Nit: 4490269
2. DatetimeIndex tests: 5232948
@jreback : sorry I had the above two commits ready in my pipeline, but I had not yet read your recent comments. I will address them later.

demonstrates loss of precision and requires no UserWarning is raised for non standard frequencies

The old `round_ns` is replaced by `round_nsint64`; `round_nsint64` is based on integer arithmetic while `round_ns` was based on floating point numbers. Rounding mode is explicitly defined by RoundTo enum class: - RoundTo.MINUS_INFTY rounds to -∞ (floor) - RountTo.PLUS_INFTY rounds to +∞ (ceil) - RoundTo.NEAREST_HALF_MINUS_INFTY rounds to nearest multiple, and breaks tie to -∞ - RoundTo.NEAREST_HALF_PLUS_INFTY rounds to nearest multiple, and breaks tie to +∞ - RoundTo.NEAREST_HALF_EVEN rounds to nearest multiple, and breaks tie to even multiple

miccoli · 2018-09-25T15:29:44Z

rebase done

jreback

looks good. @mroeschke can comment as well.

pandas/tests/indexes/datetimes/test_scalar_compat.py

mroeschke

LGTM. Agreed that .format is preferable over % for the string error formatting.

miccoli · 2018-09-26T09:38:59Z

the ci/circleci: py27_compat failure for 53c8c9b is a false negative, due to a networking problem during the build phase.

jreback

tiny typo. ping on green.

jreback · 2018-09-26T10:07:16Z

doc/source/whatsnew/v0.24.0.txt

@@ -646,6 +646,7 @@ Datetimelike
 - Bug in :class:`DatetimeIndex` subtraction that incorrectly failed to raise ``OverflowError`` (:issue:`22492`, :issue:`22508`)
 - Bug in :class:`DatetimeIndex` incorrectly allowing indexing with ``Timedelta`` object (:issue:`20464`)
 - Bug in :class:`DatetimeIndex` where frequency was being set if original frequency was ``None`` (:issue:`22150`)
+- Bug in rounding methods of :class:`DatetimeIndex` (:meth:`~DatetimeIndex.round`, :meth:`~DatetimeIndex.ceil`, :meth:`~DatetimeIndex.floor`) and :class:`Timestamp` (:meth:`~Timestamp.round`, :meth:`~Timestamp.ceil`, :meth:`~Timestamp.floor`) could give raise to loss of precision (:issue:`22591`)


raise -> rise

thanks for catching this typo

jreback · 2018-09-26T10:11:20Z

pandas/_libs/tslibs/timestamps.pyx

+        quotient[mask] += 1
+        return quotient * unit
+
+    # this should be unreachable


you could say undefined mode

miccoli · 2018-09-26T19:39:10Z

hope everything is ready for merge (green light).

jreback · 2018-09-28T11:39:58Z

lgtm. waiting till CI is un-borked to merge.

jreback · 2018-10-01T12:10:53Z

thanks @miccoli

miccoli · 2018-10-01T21:13:50Z

thanks for merging

pandas-dev#22802 imports `enum`, which was added to the built-ins in Python 3.4. The `enum34` package backports `enum` to earlier Python versions, so this should be a dependency as long as Python 2.7 is still supported.

* upstream/master: pct change bug issue 21200 (pandas-dev#21235) DOC: Fix summaries not ending with a period (pandas-dev#24190) DOC: Add back currentmodule to api.rst (pandas-dev#24232) DOC: Fixed implicit imports for whatsnew (v >= version 20.0) (pandas-dev#24199) remove enum import for PY2 compat, xref pandas-dev#22802 (pandas-dev#24170) CI: Simplify Azure Pipelines configuration files (pandas-dev#23111) REF/TST: Add more pytest idiom to indexing/multiindex/test_getitem.py (pandas-dev#24053)

…24170)

miccoli changed the title ~~Gh#22591~~ Fix Timestamp.round errors Sep 21, 2018

jbrockmendel reviewed Sep 21, 2018

View reviewed changes

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

jbrockmendel reviewed Sep 21, 2018

View reviewed changes

mroeschke requested changes Sep 21, 2018

View reviewed changes

jreback requested changes Sep 23, 2018

View reviewed changes

jreback added Datetime Numeric Operations Timezones labels Sep 23, 2018

jreback reviewed Sep 23, 2018

View reviewed changes

miccoli added 4 commits September 25, 2018 13:26

new test cases for GH#22591

fb25c21

demonstrates loss of precision and requires no UserWarning is raised for non standard frequencies

refactor: avoid 1 letter variable names, remove unreachable code

eee47a6

code review: adding docs, references, minor formatting

f6f131b

miccoli force-pushed the GH#22591 branch from 1291517 to f6f131b Compare September 25, 2018 15:26

jreback approved these changes Sep 25, 2018

View reviewed changes

pandas/tests/indexes/datetimes/test_scalar_compat.py Outdated Show resolved Hide resolved

jreback added this to the 0.24.0 milestone Sep 25, 2018

mroeschke approved these changes Sep 25, 2018

View reviewed changes

code formatting changes

53c8c9b

jreback approved these changes Sep 26, 2018

View reviewed changes

better error message and typo corrected

988e5e3

jreback merged commit f021bbc into pandas-dev:master Oct 1, 2018

miccoli mentioned this pull request Oct 5, 2018

remainder is not implemented for timedelta64 numpy/numpy#12092

Closed

effigies mentioned this pull request Dec 7, 2018

Python 2 builds breaking pybids MacPython/pandas-wheels#36

Closed

effigies mentioned this pull request Dec 7, 2018

BLD: Require enum34 for Python < 3.4 #24153

Closed

3 tasks

jreback added a commit to jreback/pandas that referenced this pull request Dec 9, 2018

remove enum import for PY2 compat, xref pandas-dev#22802

7397955

jreback mentioned this pull request Dec 9, 2018

remove enum import for PY2 compat, xref #22802 #24170

Merged

jreback added a commit to jreback/pandas that referenced this pull request Dec 11, 2018

remove enum import for PY2 compat, xref pandas-dev#22802

df29799

chris-b1 mentioned this pull request Dec 11, 2018

Timestamp.floor() rounds up when a few nanoseconds before a round time #24225

Closed

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

remove enum import for PY2 compat, xref pandas-dev#22802 (pandas-dev#…

126e0cc

…24170)

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

remove enum import for PY2 compat, xref pandas-dev#22802 (pandas-dev#…

ec4a12e

…24170)

mroeschke mentioned this pull request Sep 12, 2019

Series.dt.round fails if fraction is exactly 0.5 #28408

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Timestamp.round errors #22802

Fix Timestamp.round errors #22802

miccoli commented Sep 21, 2018 •

edited

Loading

pep8speaks commented Sep 21, 2018

codecov bot commented Sep 21, 2018 •

edited

Loading

jbrockmendel Sep 21, 2018

miccoli Sep 23, 2018

jbrockmendel Sep 21, 2018

miccoli Sep 23, 2018 •

edited

Loading

mroeschke left a comment

jreback left a comment

jreback Sep 23, 2018

jreback Sep 23, 2018

miccoli Sep 23, 2018

jreback Sep 23, 2018

jreback commented Sep 23, 2018

miccoli commented Sep 23, 2018

miccoli commented Sep 25, 2018

jreback left a comment

mroeschke left a comment

miccoli commented Sep 26, 2018 •

edited

Loading

jreback left a comment

jreback Sep 26, 2018

miccoli Sep 26, 2018

jreback Sep 26, 2018

miccoli commented Sep 26, 2018 •

edited

Loading

jreback commented Sep 28, 2018

jreback commented Oct 1, 2018

miccoli commented Oct 1, 2018

		return _floor_int64(v + u//2, u)


		def round_nsint64(values, mode: RoundTo, freq):

	# this should be unreachable
	assert False, "should never arrive here"

Fix Timestamp.round errors #22802

Fix Timestamp.round errors #22802

Conversation

miccoli commented Sep 21, 2018 • edited Loading

pep8speaks commented Sep 21, 2018

codecov bot commented Sep 21, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miccoli Sep 23, 2018 • edited Loading

Choose a reason for hiding this comment

mroeschke left a comment

Choose a reason for hiding this comment

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

jreback commented Sep 23, 2018

miccoli commented Sep 23, 2018

miccoli commented Sep 25, 2018

jreback left a comment

Choose a reason for hiding this comment

mroeschke left a comment

Choose a reason for hiding this comment

miccoli commented Sep 26, 2018 • edited Loading

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

miccoli commented Sep 26, 2018 • edited Loading

jreback commented Sep 28, 2018

jreback commented Oct 1, 2018

miccoli commented Oct 1, 2018

miccoli commented Sep 21, 2018 •

edited

Loading

codecov bot commented Sep 21, 2018 •

edited

Loading

miccoli Sep 23, 2018 •

edited

Loading

miccoli commented Sep 26, 2018 •

edited

Loading

miccoli commented Sep 26, 2018 •

edited

Loading