Docstrings, de-duplicate EAMixin/DatetimeLikeIndex __new__ code #21926
Conversation
Codecov Report
@@ Coverage Diff @@
## master #21926 +/- ##
==========================================
+ Coverage 91.96% 91.97% +<.01%
==========================================
Files 166 166
Lines 50329 50297 -32
==========================================
- Hits 46287 46261 -26
+ Misses 4042 4036 -6
Continue to review full report at Codecov.
|
gfyoung
added
Clean
ExtensionArray
pandas/core/arrays/base.py
Outdated
| @@ -634,6 +634,11 @@ class ExtensionOpsMixin(object): | |||
| """ | |||
| A base class for linking the operators to their dunder names | |||
| """ | |||
|
|
|||
| @classmethod | |||
There was a problem hiding this comment.
ideally should add doc-strings & to the top-level docs, as this is API
There was a problem hiding this comment.
OK. I think I'll revert this part of the PR as out-of-scope and do the docs bit separately.
| try: | ||
| dtype = DatetimeTZDtype.construct_from_string(dtype) | ||
| dtz = getattr(dtype, 'tz', None) | ||
| if dtz is not None: |
There was a problem hiding this comment.
hmm, does tz_compare not take None as an arg? hmm make should add the check there? as I don't think we check for None's elsewhere (not really sure about this)
There was a problem hiding this comment.
None-tz comparisons are a pretty special case that get handled separately. I’m OK with tzcompare not taking None.
|
|
||
| import pandas.core.common as com | ||
| from pandas.core.algorithms import checked_add_with_arr | ||
|
|
||
| from .base import ExtensionOpsMixin |
There was a problem hiding this comment.
can you do absolute imports
There was a problem hiding this comment.
Tried it, causes import-time errors
There was a problem hiding this comment.
i see, we import in init all of the extension arrays which import this. hmm.
| @@ -464,7 +468,10 @@ def _addsub_offset_array(self, other, op): | |||
| "{cls} not vectorized" | |||
| .format(cls=type(self).__name__), PerformanceWarning) | |||
|
|
|||
| res_values = op(self.astype('O').values, np.array(other)) | |||
There was a problem hiding this comment.
is it worth a branch here in is_extension_dtype?
There was a problem hiding this comment.
I think values_from_object is a pretty clean solution to this.
|
Thoughts? The next step here is to deal with cached_range issues. |
|
|
||
| import pandas.core.common as com | ||
| from pandas.core.algorithms import checked_add_with_arr | ||
|
|
||
| from .base import ExtensionOpsMixin |
There was a problem hiding this comment.
i see, we import in init all of the extension arrays which import this. hmm.
| try: | ||
| dtype = DatetimeTZDtype.construct_from_string(dtype) | ||
| dtz = getattr(dtype, 'tz', None) | ||
| if dtz is not None: |
| try: | ||
| dtype = DatetimeTZDtype.construct_from_string(dtype) | ||
| dtz = getattr(dtype, 'tz', None) | ||
| if dtz is not None: |
There was a problem hiding this comment.
hmm, does tz_compare not take None as an arg? hmm make should add the check there? as I don't think we check for None's elsewhere (not really sure about this)
|
@jreback since we're on a roll I'd like to push this through, get started on the next steps over the weekend. Thoughts? |
|
sure. my comments were minor. |
There's a lot of duplication in the constructors and constructor-helpers. This starts to whittle that down, writes some docstrings long the way.
Also use
ExtensionOpsMixinto define comparison operators on the EAMixin classes.We determined that the DatetimeIndex._local_timestamps method had an unecessary monotonicy check, so took that out.