[Bug] Fix various DatetimeIndex comparison bugs by jbrockmendel · Pull Request #22074 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation21 Commits10 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Will have to see which issues this closes. This is also a precursor to upcoming PR that dispatches DataFrame ops to Series (which in turn dispatches to Index).
Fixes #7830 (the OP's example is already fixed, this fixes examples in the comments to that issue)
Fixes DTI half of #19804
- closes Equality comparison raises exception #7830
- tests added / passed
- passes
git diff upstream/master -u -- "*.py" | flake8 --diff - whatsnew entry
| raise TypeError('%s type object %s' % |
|---|
| (type(other), str(other))) |
| if is_datetimelike(other): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This previously called self._assert_tzawareness_compat(other) for other with timedelta64-dtype
(Really, is_datetimelike is a footgun)
| @@ -475,7 +479,7 @@ Numeric |
|---|
| - Bug in :class:`Series` ``__rmatmul__`` doesn't support matrix vector multiplication (:issue:`21530`) |
| - Bug in :func:`factorize` fails with read-only array (:issue:`12813`) |
| - Fixed bug in :func:`unique` handled signed zeros inconsistently: for some inputs 0.0 and -0.0 were treated as equal and for some inputs as different. Now they are treated as equal for all inputs (:issue:`21866`) |
| - |
| - Bug in :class:`Series` comparison against datetime-like scalars and arrays (:issue:`22074`) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not where this line was intended, will move.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I take it back; this is where it belongs.
| with pytest.raises(TypeError): |
|---|
| rng > other |
| with pytest.raises(TypeError): |
| rng >= other |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anything worth checking error message-wise?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are pretty scattershot at the moment. I'll give some thought to how that can be addressed.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice fixtures. need to narrow down existing issues which it closes.
| @@ -99,31 +100,40 @@ def wrapper(self, other): |
|---|
| meth = getattr(dtl.DatetimeLikeArrayMixin, opname) |
| if isinstance(other, (datetime, np.datetime64, compat.string_types)): |
| if isinstance(other, datetime): |
| if isinstance(other, (datetime, np.datetime64)): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we do this all over the place, thing about making a type for this (for in internal use only), and/or maybe a is_datetime_scalar or something, just so we are consistent
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got a branch going looking at core.dtypes.common (inspired by noticing the footgun status if is_datetimelike), will look at this there so I can get all the places it is used in one swoop.
| @@ -152,6 +162,10 @@ class DatetimeArrayMixin(dtl.DatetimeLikeArrayMixin): |
|---|
| 'is_year_end', 'is_leap_year'] |
| _object_ops = ['weekday_name', 'freq', 'tz'] |
| # dummy attribute so that datetime.__eq__(DatetimeArray) defers |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh? why don't you just implement and raise NotIMplemented?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is is a quirk of the stdlib datetime implementation. datetime == other will return NotImplemented instead of raising TypeError iff other has a timetuple attribute.
| @@ -275,6 +275,20 @@ def test_comparison_tzawareness_compat(self, op): |
|---|
| with pytest.raises(TypeError): |
| op(ts, dz) |
| @pytest.mark.parametrize('op', [operator.eq, operator.ne, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pretty sure we have fixtures for this
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a fixture for the names ["__eq__", ...]. This will go on the list of things I'd like to standardize after collecting arithmetic tests in one place.
need to narrow down existing issues which it closes.
AFAICT #7830 is the only issue this closes on its own. BUT after this is done we can make more DataFrame ops operate column-wise (orthogonal to #22019 because DataFrame ops are a hive of scum and villainy), and that closes a ton of issues.
update That follow-up will be made much less verbose if preceeded by #22068.
AFAICT #7830 is the only issue this closes on its own. BUT after this is done we can make more DataFrame ops operate column-wise (orthogonal to #22019 because DataFrame ops are a hive of scum and villainy), and that closes a ton of issues.
update That follow-up will be made much less verbose if preceeded by #22068.
you wnt to merge this first then #22068 ?
This and #22068 should be orthogonal, either order is fine.
| @@ -496,6 +496,9 @@ Datetimelike |
|---|
| - Fixed bug where two :class:`DateOffset` objects with different ``normalize`` attributes could evaluate as equal (:issue:`21404`) |
| - Fixed bug where :meth:`Timestamp.resolution` incorrectly returned 1-microsecond ``timedelta`` instead of 1-nanosecond :class:`Timedelta` (:issue:`21336`,:issue:`21365`) |
| - Bug in :func:`to_datetime` that did not consistently return an :class:`Index` when ``box=True`` was specified (:issue:`21864`) |
| - Bug in :class:`DatetimeIndex` comparisons where string comparisons incorrectly raises ``TypeError`` (:issue:`22074`) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should one be marked #7830 ?
tiny comment, not sure if worth changing
tiny comment, not sure if worth changing
Let's roll that into one of the cleanup-followups. I'm eager to do the fix-all-the-bugs one that comes after this.
dberenbaum pushed a commit to dberenbaum/pandas that referenced this pull request
Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request