-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Make DTI[tz]._values and Series[tz]._values return DTA #24534
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
Conversation
lgtm. |
Codecov Report
@@ Coverage Diff @@
## master #24534 +/- ##
==========================================
+ Coverage 31.88% 31.88% +<.01%
==========================================
Files 166 166
Lines 52427 52436 +9
==========================================
+ Hits 16714 16721 +7
- Misses 35713 35715 +2
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #24534 +/- ##
==========================================
+ Coverage 31.88% 31.88% +<.01%
==========================================
Files 166 166
Lines 52427 52436 +9
==========================================
+ Hits 16714 16721 +7
- Misses 35713 35715 +2
Continue to review full report at Codecov.
|
gentle ping |
thanks @jbrockmendel |
@@ -340,7 +346,7 @@ def _values(self): | |||
# tz-naive -> ndarray | |||
# tz-aware -> DatetimeIndex | |||
if self.tz is not None: | |||
return self | |||
return self._eadata |
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.
This change is obsolete in #24024 since we always return a DatetimeArray.
* upstream/master: Make DTI[tz]._values and Series[tz]._values return DTA (pandas-dev#24534) CLN: Refactor some sorting code in Index set operations (pandas-dev#24533) Run isort (pandas-dev#24530) CI: fix db usage in CI (pandas-dev#24529)
@@ -316,6 +316,12 @@ def _simple_new(cls, values, name=None, freq=None, tz=None, dtype=None): | |||
we require the we have a dtype compat for the values | |||
if we are passed a non-dtype compat, then coerce using the constructor | |||
""" | |||
if isinstance(values, DatetimeArray): |
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.
Do we have anywhere that passes a DatetimeArray here and additional keyword arguments?
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.
I'm pretty sure we do. I'll double-check and post
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.
Yes. At the very least two tests in test_multilevel
result = self._data.internal_values() | ||
if isinstance(result, DatetimeIndex): | ||
result = result._eadata | ||
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.
this should also be removeable in 24024
broken off of #24024, cc @jreback @TomAugspurger
I think the edits in core.dtypes.concat are unrelated, but they are correct regardless and easy to trim a few more lines off the diff.
The edit in tests.indexing.test_coercion was needed during troubleshooting, decided to keep it.