WIP: Dispatch Series comparison methods to Index implementations #19288
Conversation
| # kludge; DatetimeIndex refuses to compare against None or | ||
| # datetime.date; until the "right" behavior is resolved, we cast | ||
| # these types here to types that DatetimeIndex understand. | ||
| if type(other) is datetime.date: |
There was a problem hiding this comment.
use isinstance.
what do you mean refuses to compare?
There was a problem hiding this comment.
use isinstance.
Will change, but note the isinstance version is pretty verbose: if isinstance(other, datetime.date) and not isinstance(other, datetime.datetime).
what do you mean refuses to compare?
DatetimeIndex comparison with a) datetime.date or b) None raises TypeError; Series casts to datetime and NaT, respectively.
dti = pd.date_range('2016-01-01', periods=2)
ser = pd.Series(dti)
>>> dti == datetime.date(2016, 1, 1)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "pandas/core/indexes/datetimes.py", line 119, in wrapper
other = _ensure_datetime64(other)
File "pandas/core/indexes/datetimes.py", line 145, in _ensure_datetime64
raise TypeError('%s type object %s' % (type(other), str(other)))
TypeError: <type 'datetime.date'> type object 2016-01-01
>>> ser == datetime.date(2016, 1, 1)
0 True
1 False
dtype: bool
>>> dti != None
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "pandas/core/indexes/datetimes.py", line 119, in wrapper
other = _ensure_datetime64(other)
File "pandas/core/indexes/datetimes.py", line 145, in _ensure_datetime64
raise TypeError('%s type object %s' % (type(other), str(other)))
TypeError: <type 'NoneType'> type object None
>>> ser != None
0 True
1 True
dtype: bool
So really one of these two behaviors should be chosen as canonical and the other changed (and tested), at which point these shims will be unnecessary.
There was a problem hiding this comment.
these comparison should work for both DTI and None. These work for scalars as well already.
There was a problem hiding this comment.
I am totally on board with the "DTI comparison should behave like a vectorized Timestamp comparison" approach, but it isn't 100% straightforward:
For datetime.date comparison, Timestamp.__eq__ is always False and Timestamp.__ne__ is always True; lt, le, gt, ge all raise TypeError.
dti = pd.date_range('2016-01-01', periods=2)
ts = dti[0]
dt = ts.to_pydatetime()
d = dt.date()
>>> ts == d
False
>>> ts < d
File "pandas/_libs/tslibs/timestamps.pyx", line 121, in pandas._libs.tslibs.timestamps._Timestamp.__richcmp__
raise TypeError('Cannot compare type %r with type %r' %
TypeError: Cannot compare type 'Timestamp' with type 'date'
Same behavior for None, i.e. it is not treated being equivalent to pd.NaT.
There was a problem hiding this comment.
hmm, I would prefer a patch independetly of this one to fix these 2 cases, these already work with DTI
There was a problem hiding this comment.
#19301 is intended to do that. Did you have something else in mind?
| # these types here to types that DatetimeIndex understand. | ||
| if type(other) is datetime.date: | ||
| other = datetime.datetime(other.year, other.month, other.day) | ||
| elif other is None: |
There was a problem hiding this comment.
why is this None check needed? do we not do this in the op comparison directly?
There was a problem hiding this comment.
and I don't like this code path at all, meaning you are special casing things, which IMHO should be handle inside the DTI comparisons.
There was a problem hiding this comment.
Yes; the next step is to put this on the back-burner and fix the DTI comparisons.
Codecov Report
@@ Coverage Diff @@
## master #19288 +/- ##
==========================================
+ Coverage 91.56% 91.56% +<.01%
==========================================
Files 148 148
Lines 48888 48881 -7
==========================================
- Hits 44765 44760 -5
+ Misses 4123 4121 -2
Continue to review full report at Codecov.
|
|
I can’t replicate appveyor error locally. Something windows specific? |
|
Closing until #19301 |
Same thing we've been doing for arithmetic operations, now moving on to comparison methods. Based on some trial-and-error, it seems like there are some inconsistencies to be worked out for the categorical dtype case. This PR:
Seriescomparisons to their respectiveIndexsubclass implementationsDatetimeIndex._assert_tz_awareness_compat, which has already been broken out into Fix tzawareness_compat for DatetimeIndex comparisons with NaT #19276.datetime.dateandNonecase thatSeriestreats differently fromDatetimeIndex; decisions should be made about those so these kludges can be removed.# temp workaround until fixing GH 13637for comparison withPeriodIndexthat appears to no longer be needed.SeriesandIndexclasses. (probably needs tests)_constructoris called, also discussed in Series comparison ops __finalize__ inconsistent #19271.