Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

disallow normalize=True with Tick classes #21427

Merged
merged 6 commits into from Jun 14, 2018

Conversation

Projects
None yet
3 participants
@jbrockmendel
Copy link
Member

commented Jun 11, 2018

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 #21434
  • tests added/passed
  • passes flake8
  • whatsnew note
@codecov

This comment has been minimized.

Copy link

commented Jun 11, 2018

Codecov Report

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

Impacted file tree graph

@@            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.

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

This comment has been minimized.

Copy link
@gfyoung

gfyoung Jun 11, 2018

Member

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

This comment has been minimized.

Copy link
@gfyoung

gfyoung Jun 13, 2018

Member

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

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

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

This comment has been minimized.

Copy link
@gfyoung

gfyoung Jun 11, 2018

Member

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

This comment has been minimized.

Copy link
@gfyoung

gfyoung Jun 11, 2018

Member

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

@gfyoung

This comment has been minimized.

Copy link
Member

commented Jun 11, 2018

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

@jreback
Copy link
Contributor

left a comment

is there an associated issue?

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

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

This comment has been minimized.

Copy link
@jreback

jreback Jun 12, 2018

Contributor

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

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Jun 12, 2018

Author Member

Will do.

@jreback jreback added this to the 0.24.0 milestone Jun 12, 2018

@gfyoung

This comment has been minimized.

Copy link
Member

commented Jun 12, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2018

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

can u make a sub section with an example in whatsnew

@jbrockmendel

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2018

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


.. ipython:: python

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

This comment has been minimized.

Copy link
@jreback

jreback Jun 13, 2018

Contributor

show ts and tic here

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Jun 13, 2018

Author Member

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


.. code-block:: ipython

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

This comment has been minimized.

Copy link
@jreback

jreback Jun 13, 2018

Contributor

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


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

This comment has been minimized.

Copy link
@jreback

jreback Jun 13, 2018

Contributor

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

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Jun 13, 2018

Author Member

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

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

.. _whatsnew_0240.api.datetimelike:

This comment has been minimized.

Copy link
@jreback

jreback Jun 13, 2018

Contributor

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

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

-

This comment has been minimized.

Copy link
@jreback

jreback Jun 13, 2018

Contributor

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

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.

This comment has been minimized.

Copy link
@jreback

jreback Jun 13, 2018

Contributor

add the issue number here

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

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jreback

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2018

thanks @jbrockmendel

alimcmaster1 added a commit to alimcmaster1/pandas that referenced this pull request Jun 28, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.