nat division by timedelta #17955
nat division by timedelta #17955
Conversation
Codecov Report
@@ Coverage Diff @@
## master #17955 +/- ##
==========================================
- Coverage 91.23% 91.22% -0.02%
==========================================
Files 163 163
Lines 50113 50113
==========================================
- Hits 45723 45714 -9
- Misses 4390 4399 +9
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #17955 +/- ##
==========================================
- Coverage 91.24% 91.23% -0.02%
==========================================
Files 163 163
Lines 50168 50168
==========================================
- Hits 45778 45769 -9
- Misses 4390 4399 +9
Continue to review full report at Codecov.
|
doc/source/whatsnew/v0.21.0.txt
Outdated
| @@ -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`) | |||
| - ``NaT`` division with :class:`datetime.timedelta` will now return NaN instead of raising (:issue:`17876`) | |||
There was a problem hiding this comment.
Also, what is the rationale for returning NaN ? Why not return NaT instead?
There was a problem hiding this comment.
It also occurs to me that you might not need a whatsnew given that this is internal.
There was a problem hiding this comment.
Double-tick the NaN.
Just pushed a fix.
Also, what is the rationale for returning
NaN? Why not returnNaTinstead?
This is just taking behavior that currently applies to Timedelta and applying the same to timedelta.
There was a problem hiding this comment.
Hmm...seems a little non-intuitive to me, but then again, I'm not going to push changing the norm without getting feedback from other maintainers:
There was a problem hiding this comment.
I think the idea is that in this context NaT is ducking as a Timedelta, so Y = NaT / Timedelta(X) is solving for Timedelta(X) * Y = NaT --> Y = np.nan
There was a problem hiding this comment.
Ah, I see. Right, that makes sense. Leave it be then.
There was a problem hiding this comment.
Yes, that seems to make sense to me, similar as:
In [5]: np.nan / 5
Out[5]: nan
And also this is how pd.NaT / pd.Timedelta(..) is already working? In that case I would rather call it a bug fix.
There was a problem hiding this comment.
And also this is how pd.NaT / pd.Timedelta(..) is already working?
Yes.
|
will be for 0.21.1, we haven't pushed the whatsnew yet, so will need to update then. |
OK. Does that mean I need to something now or just be patient? |
|
@jbrockmendel : Just hold on for the time being. This PR is good to go otherwise I think. We'll ping you once it's added, or if we don't, ping us about it. |
doc/source/whatsnew/v0.21.0.txt
Outdated
| @@ -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`) | |||
| - ``NaT`` division with :class:`datetime.timedelta` will now return ``NaN`` instead of raising (:issue:`17876`) | |||
There was a problem hiding this comment.
so will move to 0.21.1 when its available
There was a problem hiding this comment.
can you move to 0.22.0 other api changes
There was a problem hiding this comment.
Done. Out of curiosity, why 0.22.0 instead of 0.21.1?
|
@jbrockmendel if you would rebase |
|
ok ping on green. |
|
thanks |
xref #17876
Does the whatsnew note still go in 0.20.0.txt?
git diff upstream/master -u -- "*.py" | flake8 --diff