-
-
Notifications
You must be signed in to change notification settings - Fork 18.7k
BUG: Fix index order for Index.intersection() #15583
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
Changes from 1 commit
c12bb3f
c2a8dc3
a4ead99
e7bcd28
d9e29f8
1197b99
784fe75
b977278
ec836bd
047b513
c96306d
f0d9d03
ef2581e
3c200fe
33eb740
654288b
5bf1508
9e39794
968c7f1
8d2e9cc
73df69e
64e86a4
2d4e143
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1203,7 +1203,8 @@ def intersection(self, other): | |
not other.offset.isAnchored() or | ||
(not self.is_monotonic or not other.is_monotonic)): | ||
result = Index.intersection(self, other) | ||
result = self._simple_new(result._values, name=result.name, tz=result.tz) | ||
result = self._simple_new(result._values, name=result.name, | ||
tz=result.tz) | ||
if result.freq is None: | ||
result.offset = to_offset(result.inferred_freq) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this change related to the PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, as the simple result of Index.intersection inherits all attributes from other, and this is not the expected result. Therefore, it is necessary to create a new DatetimeIndex using only three of the Index.intersection attributes: _values, name and tz. |
||
return result | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use
_shallow_copy
rather than_simple_new
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I use _shallow_copy, the result will inherit all self attributes (e.g., freq) and this might not be the expected result. For example, this test would fail:
because result would inherit self freq = 'D'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe you misunderstand, simply pass the new freq
result = self._shallow_copy(...., freq=....)
We don't use
_simple_new
at all (outside of a few specific places), this is not one of them. This is all documented next to these functions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might I simply use the constructor DatetimeIndex()? I ask this because I think the result freq is not known in advance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, use
_shallow_copy
withfreq=None
, which will not infer it, and then you can reset the freq after