disallow normalize=True with Tick classes #21427

jbrockmendel · 2018-06-11T17:53:10Z

The problem: allowing Tick objects with normalize=True causes addition to lose monotonicity/associativity

ts = pd.Timestamp.now()
tick = pd.offsets.Minute(n=4, normalize=True)
>>> ts
Timestamp('2018-06-11 10:50:14.419655')
>>> ts + tick
Timestamp('2018-06-11 00:00:00')

closes Tick with normalize=True should be disallowed #21434
tests added/passed
passes flake8
whatsnew note

codecov · 2018-06-11T19:11:28Z

Codecov Report

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

@@            Coverage Diff             @@
##           master   #21427      +/-   ##
==========================================
+ Coverage    91.9%    91.9%   +<.01%     
==========================================
  Files         153      153              
  Lines       49606    49608       +2     
==========================================
+ Hits        45589    45591       +2     
  Misses       4017     4017

Flag	Coverage Δ
#multiple	`90.3% <100%> (ø)`	⬆️
#single	`41.89% <50%> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/tseries/offsets.py	`97.25% <100%> (ø)`	⬆️

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 576d5c6...d15d137. Read the comment docs.

gfyoung · 2018-06-11T21:36:17Z

pandas/tseries/offsets.py

@@ -2219,6 +2219,9 @@ class Tick(SingleConstructorOffset):
    def __init__(self, n=1, normalize=False):
        # TODO: do Tick classes with normalize=True make sense?


It seems like this was a broader API design question. Depending on how things go, creating an issue to "close" with this PR might be appropriate.

cc @jreback

@jbrockmendel : Could you remove this comment? Looks like your fix is here to say. 😄

gfyoung · 2018-06-11T21:37:25Z

doc/source/whatsnew/v0.24.0.txt

@@ -112,7 +112,7 @@ Timezones
 Offsets
 ^^^^^^^

-
+- :class:`Tick` subclasses can no longer be created with the argument `normalize=True` (:issue:`????`)


In cases like these, reference the PR itself (unless an issue is created as pointed out here).

The same goes for the test case (i.e. add a comment referencing the PR).

gfyoung · 2018-06-11T23:49:45Z

@jbrockmendel : If this is the correct way to fix (I'm not 100% sure @jreback ), then your PR is good to go.

jreback

is there an associated issue?

jreback · 2018-06-12T00:19:54Z

doc/source/whatsnew/v0.24.0.txt

@@ -112,7 +112,7 @@ Timezones
 Offsets
 ^^^^^^^

-
+- :class:`Tick` subclasses can no longer be created with the argument `normalize=True` (:issue:`21427`)


double backticks here. I would be a little more informative (say for example Minute, Second).

gfyoung · 2018-06-12T00:21:23Z

is there an associated issue?

@jreback : No, there isn't AFAIK. I suggested to @jbrockmendel that one be opened only if there was disagreement about the fix.

jbrockmendel · 2018-06-12T00:45:32Z

Opened #21434, extends the example given in the OP and mentions that Day could be a special case (though I'd prefer it not)

jreback · 2018-06-12T00:48:39Z

@jbrockmendel you didn’t need to make an issue, but ok

can u make a sub section with an example in whatsnew

jbrockmendel · 2018-06-12T01:04:58Z

Just added a whatsnew section, not sure it adds much on top of the one-line note (also fleshed out)

jreback · 2018-06-13T10:41:22Z

doc/source/whatsnew/v0.24.0.txt

+
+.. ipython:: python
+
+    ts = pd.Timestamp('2018-06-11 18:01:14')


show ts and tic here

Will do. Does showing the tic here work given that the line that defines it will raise in the new version?

jreback · 2018-06-13T10:41:41Z

doc/source/whatsnew/v0.24.0.txt

+
+.. code-block:: ipython
+
+    In [2]: ts = pd.Timestamp('2018-06-11 18:01:14')


you don't need to repeat ts & tic here, just the calculations

jreback · 2018-06-13T10:41:55Z

doc/source/whatsnew/v0.24.0.txt

+
+    In [5]: ts + tic + tic + tic == ts + (tic + tic + tic)
+    Out [5]: False
+


add the Current behavior via an ipython block (obviously w/o normalize passed)

OK. And just the same ts + tic + tic + tic == ts + (tic + tic + tic) demonstration? With normalize=False that behavior is unchanged.

jreback · 2018-06-13T10:42:32Z

doc/source/whatsnew/v0.24.0.txt

@@ -26,6 +26,33 @@ Backwards incompatible API changes

 .. _whatsnew_0240.api.datetimelike:



add a ref to this section (this is a new one, like: _whatsnew_0240.api.datetimelike.normalize )

jreback · 2018-06-13T10:43:21Z

doc/source/whatsnew/v0.24.0.txt

@@ -112,7 +139,7 @@ Timezones
 Offsets
 ^^^^^^^

-


if you have a subsection, you don't need a note, the subsection IS the note.

jreback · 2018-06-13T10:43:33Z

doc/source/whatsnew/v0.24.0.txt

+Creating a ``Tick`` object (:class:``Day``, :class:``Hour``, :class:``Minute``,
+:class:``Second``, :class:``Milli``, :class:``Micro``, :class:``Nano``) with
+`normalize=True` is no longer supported.  This prevents unexpected behavior
+where addition could fail to be monotone or associative.


add the issue number here

…cknorm

jreback · 2018-06-14T10:18:33Z

thanks @jbrockmendel

disallow normalize=True with Tick classes

a9cc9f0

gfyoung added Bug Timedelta Timedelta data type labels Jun 11, 2018

gfyoung reviewed Jun 11, 2018

View reviewed changes

update with GH references

b4cbaa9

jreback requested changes Jun 12, 2018

View reviewed changes

jreback added this to the 0.24.0 milestone Jun 12, 2018

jreback added the Error Reporting Incorrect or improved errors from pandas label Jun 12, 2018

jbrockmendel mentioned this pull request Jun 12, 2018

Tick with normalize=True should be disallowed #21434

Closed

jbrockmendel added 2 commits June 11, 2018 17:49

flesh out doc note

6a28813

Whatsnew section

053aa14

jreback requested changes Jun 13, 2018

View reviewed changes

jbrockmendel added 2 commits June 13, 2018 07:58

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

44a2cde

…cknorm

requested edits, mostly whatsnew

d15d137

jreback approved these changes Jun 14, 2018

View reviewed changes

jreback merged commit fd121ed into pandas-dev:master Jun 14, 2018

david-liu-brattle-1 pushed a commit to david-liu-brattle-1/pandas that referenced this pull request Jun 18, 2018

disallow normalize=True with Tick classes (pandas-dev#21427)

144223d

jbrockmendel mentioned this pull request Jun 22, 2018

Tick DateOffset Normalize whatsnew entry has an error #21564

Closed

2 tasks

jbrockmendel deleted the ticknorm branch June 22, 2018 03:27

jorisvandenbossche mentioned this pull request Jun 26, 2018

Day(normalize=True) gives ValueError: Tick offset with normalize=True are not allowed. #21633

Closed

Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018

disallow normalize=True with Tick classes (pandas-dev#21427)

a7e6b10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

disallow normalize=True with Tick classes #21427

disallow normalize=True with Tick classes #21427

jbrockmendel commented Jun 11, 2018 •

edited

Loading

codecov bot commented Jun 11, 2018 •

edited

Loading

gfyoung Jun 11, 2018 •

edited

Loading

gfyoung Jun 13, 2018

gfyoung Jun 11, 2018

gfyoung Jun 11, 2018

gfyoung commented Jun 11, 2018

jreback left a comment

jreback Jun 12, 2018

jbrockmendel Jun 12, 2018

gfyoung commented Jun 12, 2018

jbrockmendel commented Jun 12, 2018

jreback commented Jun 12, 2018

jbrockmendel commented Jun 12, 2018

jreback Jun 13, 2018

jbrockmendel Jun 13, 2018

jreback Jun 13, 2018

jreback Jun 13, 2018

jbrockmendel Jun 13, 2018

jreback Jun 13, 2018

jreback Jun 13, 2018

jreback Jun 13, 2018

jreback commented Jun 14, 2018

		@@ -2219,6 +2219,9 @@ class Tick(SingleConstructorOffset):
		def __init__(self, n=1, normalize=False):
		# TODO: do Tick classes with normalize=True make sense?


		.. code-block:: ipython

		In [2]: ts = pd.Timestamp('2018-06-11 18:01:14')


		In [5]: ts + tic + tic + tic == ts + (tic + tic + tic)
		Out [5]: False

		@@ -26,6 +26,33 @@ Backwards incompatible API changes

		.. _whatsnew_0240.api.datetimelike:

@@ @@ -112,7 +139,7 @@ Timezones @@
               Offsets
               ^^^^^^^
-              -

disallow normalize=True with Tick classes #21427

disallow normalize=True with Tick classes #21427

Conversation

jbrockmendel commented Jun 11, 2018 • edited Loading

codecov bot commented Jun 11, 2018 • edited Loading

Codecov Report

gfyoung Jun 11, 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 commented Jun 11, 2018

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

gfyoung commented Jun 12, 2018

jbrockmendel commented Jun 12, 2018

jreback commented Jun 12, 2018

jbrockmendel commented Jun 12, 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

jreback commented Jun 14, 2018

jbrockmendel commented Jun 11, 2018 •

edited

Loading

codecov bot commented Jun 11, 2018 •

edited

Loading

gfyoung Jun 11, 2018 •

edited

Loading