Revert change to comparison op with datetime.date objects #21361
Conversation
TomAugspurger
added
Datetime
Codecov Report
@@ Coverage Diff @@
## master #21361 +/- ##
==========================================
+ Coverage 91.89% 91.89% +<.01%
==========================================
Files 153 153
Lines 49579 49590 +11
==========================================
+ Hits 45559 45570 +11
Misses 4020 4020
Continue to review full report at Codecov.
|
| @@ -1197,8 +1200,35 @@ def wrapper(self, other, axis=None): | |||
| if is_datetime64_dtype(self) or is_datetime64tz_dtype(self): | |||
| # Dispatch to DatetimeIndex to ensure identical | |||
| # Series/Index behavior | |||
| if (isinstance(other, datetime.date) and | |||
| not isinstance(other, datetime.datetime)): | |||
There was a problem hiding this comment.
is this check needed? (I would not expect it ever to be true if it is a datetime.date ?
There was a problem hiding this comment.
Apparently datetime.datetime subclasses datetime.date
There was a problem hiding this comment.
rather than inlineing this, I think would be better to move to a module level function, maybe
other = maybe_coerce_other(other, op)
There was a problem hiding this comment.
You mean just the new bit I added, right?
I'm inclined to leave it here since we'll just be deleting it soon (is 0.24.0 too soon?).
There was a problem hiding this comment.
Apparently datetime.datetime subclasses datetime.date
Ah, only checked the other way around :-)
Yeah, if this is only temporary code I would just leave it here
There was a problem hiding this comment.
ok, is there an issue to fix this for 0.24.0? (can you add a TODO mention here)
doc/source/whatsnew/v0.23.1.txt
Outdated
|
|
||
| .. code-block:: python | ||
|
|
||
| >>> pd.Series(pd.date_range('2017', periods=2)) > datetime.date(2017, 1, 1) |
There was a problem hiding this comment.
I am not sure this example is still needed? (it's kind of repetition of above? and the note is already quite long)
There was a problem hiding this comment.
yeah I agree this is too much, the first part is great!
doc/source/whatsnew/v0.23.1.txt
Outdated
| @@ -10,12 +10,70 @@ and bug fixes. We recommend that all users upgrade to this version. | |||
| :local: | |||
| :backlinks: none | |||
|
|
|||
| .. _whatsnew_0231.enhancements: | |||
There was a problem hiding this comment.
left-over from rebase
|
|
||
| .. _whatsnew_0231.fixed_regressions: | ||
|
|
||
| Fixed Regressions | ||
| ~~~~~~~~~~~~~~~~~ | ||
|
|
||
| **Comparing Series with datetime.date** | ||
|
|
||
| We've reverted a 0.23.0 change to comparing a :class:`Series` holding datetimes and a ``datetime.date`` object (:issue:`21152`). |
There was a problem hiding this comment.
I'm not sure what you mean here.
There was a problem hiding this comment.
oh was just saying that users talk about dtypes, rather than Series holding datetimes. no big deal really.
| **Comparing Series with datetime.date** | ||
|
|
||
| We've reverted a 0.23.0 change to comparing a :class:`Series` holding datetimes and a ``datetime.date`` object (:issue:`21152`). | ||
| In pandas 0.22 and earlier, comparing a Series holding datetimes and ``datetime.date`` objects would coerce the ``datetime.date`` to a datetime before comapring. |
There was a problem hiding this comment.
use double back ticks around Series (or :class`Series`
|
|
||
| We've reverted a 0.23.0 change to comparing a :class:`Series` holding datetimes and a ``datetime.date`` object (:issue:`21152`). | ||
| In pandas 0.22 and earlier, comparing a Series holding datetimes and ``datetime.date`` objects would coerce the ``datetime.date`` to a datetime before comapring. | ||
| This was inconsistent with Python, NumPy, and :class:`DatetimeIndex`, which never consider a datetime and ``datetime.date`` equal. |
There was a problem hiding this comment.
I think we should double backtick on Python, NumPy
| In pandas 0.22 and earlier, comparing a Series holding datetimes and ``datetime.date`` objects would coerce the ``datetime.date`` to a datetime before comapring. | ||
| This was inconsistent with Python, NumPy, and :class:`DatetimeIndex`, which never consider a datetime and ``datetime.date`` equal. | ||
|
|
||
| In 0.23.0, we unified operations between DatetimeIndex and Series, and in the process changed comparisons between a Series of datetimes and ``datetime.date`` without warning. |
There was a problem hiding this comment.
double backticks on DTI & Series
doc/source/whatsnew/v0.23.1.txt
Outdated
|
|
||
| .. code-block:: python | ||
|
|
||
| >>> pd.Series(pd.date_range('2017', periods=2)) > datetime.date(2017, 1, 1) |
There was a problem hiding this comment.
yeah I agree this is too much, the first part is great!
| @@ -1197,8 +1200,35 @@ def wrapper(self, other, axis=None): | |||
| if is_datetime64_dtype(self) or is_datetime64tz_dtype(self): | |||
| # Dispatch to DatetimeIndex to ensure identical | |||
| # Series/Index behavior | |||
| if (isinstance(other, datetime.date) and | |||
| not isinstance(other, datetime.datetime)): | |||
There was a problem hiding this comment.
rather than inlineing this, I think would be better to move to a module level function, maybe
other = maybe_coerce_other(other, op)
jorisvandenbossche
changed the title
|
@TomAugspurger This is ready? |
|
IMO, yes. I didn't change the quoting in the docs, since #19750 hasn't been resolved (just replied there). |
(cherry picked from commit d79203a)
git diff upstream/master -u -- "*.py" | flake8 --diffcc @jbrockmendel, @innominate227
FYI, @jorisvandenbossche the whatsnew will conflict with your other PR