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

[Bug] Fix various DatetimeIndex comparison bugs #22074

Merged
merged 10 commits into from Aug 1, 2018

Conversation

Projects
None yet
3 participants
@jbrockmendel
Copy link
Member

commented Jul 27, 2018

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 #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):

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Jul 27, 2018

Author Member

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`)

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Jul 27, 2018

Author Member

Not where this line was intended, will move.

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Jul 27, 2018

Author Member

I take it back; this is where it belongs.

with pytest.raises(TypeError):
rng > other
with pytest.raises(TypeError):
rng >= other

This comment has been minimized.

Copy link
@gfyoung

gfyoung Jul 27, 2018

Member

Anything worth checking error message-wise?

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Jul 27, 2018

Author Member

These are pretty scattershot at the moment. I'll give some thought to how that can be addressed.

@codecov

This comment has been minimized.

Copy link

commented Jul 27, 2018

Codecov Report

Merging #22074 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #22074      +/-   ##
==========================================
- Coverage   92.07%   92.06%   -0.01%     
==========================================
  Files         170      170              
  Lines       50688    50701      +13     
==========================================
+ Hits        46671    46679       +8     
- Misses       4017     4022       +5
Flag Coverage Δ
#multiple 90.47% <100%> (-0.01%) ⬇️
#single 42.33% <45.83%> (+0.01%) ⬆️
Impacted Files Coverage Δ
pandas/core/ops.py 96.15% <100%> (+0.03%) ⬆️
pandas/core/arrays/datetimes.py 95.52% <100%> (+0.06%) ⬆️
pandas/core/arrays/datetimelike.py 94.02% <0%> (-1.04%) ⬇️
pandas/core/indexes/datetimes.py 95.54% <0%> (-0.14%) ⬇️

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 26b3e7d...3ac9102. Read the comment docs.

@jreback
Copy link
Contributor

left a comment

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)):

This comment has been minimized.

Copy link
@jreback

jreback Jul 28, 2018

Contributor

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

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Jul 28, 2018

Author Member

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

This comment has been minimized.

Copy link
@jreback

jreback Jul 28, 2018

Contributor

huh? why don't you just implement and raise NotIMplemented?

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Jul 28, 2018

Author Member

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,

This comment has been minimized.

Copy link
@jreback

jreback Jul 28, 2018

Contributor

pretty sure we have fixtures for this

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Jul 28, 2018

Author Member

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.

@jreback

This comment has been minimized.

Copy link
Contributor

commented Jul 28, 2018

pls rebase

@jbrockmendel

This comment has been minimized.

Copy link
Member Author

commented Jul 28, 2018

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.

@jreback

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2018

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 ?

@jbrockmendel

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2018

This and #22068 should be orthogonal, either order is fine.

@jbrockmendel jbrockmendel referenced this pull request Jul 30, 2018

Open

[DEPR] Assorted footguns in dtype checks #22137

1 of 6 tasks complete
@@ -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`)

This comment has been minimized.

Copy link
@jreback

jreback Jul 31, 2018

Contributor

should one be marked #7830 ?

@jreback jreback added this to the 0.24.0 milestone Jul 31, 2018

@jreback

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2018

tiny comment, not sure if worth changing

@jbrockmendel

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2018

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.

@jreback jreback merged commit 8d5c51b into pandas-dev:master Aug 1, 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 Aug 1, 2018

thxs

dberenbaum added a commit to dberenbaum/pandas that referenced this pull request Aug 3, 2018

@jbrockmendel jbrockmendel deleted the jbrockmendel:dtcmp branch Aug 8, 2018

alimcmaster1 added a commit to alimcmaster1/pandas that referenced this pull request Aug 12, 2018

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

@jbrockmendel jbrockmendel referenced this pull request Oct 24, 2018

Open

DataFrame vs Series vs Index arithmetic Roundup #18824

37 of 59 tasks complete
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.