REF: move range-generation functions to EA mixin classes #22016

jbrockmendel · 2018-07-21T21:00:40Z

With the DatetimeArray range-generating functions moved, we are finally able to move shift, and in turn __add__, __sub__, etc.

Two non-trivial changes made during the move process, see inline comments.

Upcoming commits will add docstrings and port tests. ATM many tests are a PITA bc the Array contructors don't know how to handle lists of strings. Trying to find a way to implement that in __new__ without having it become a total mess.

pep8speaks · 2018-07-21T21:00:50Z

Hello @jbrockmendel! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on July 25, 2018 at 19:12 Hours UTC

jbrockmendel · 2018-07-21T21:02:50Z

pandas/core/arrays/datetimes.py

+                start = start.tz_localize(tz)
+                end = end.tz_localize(tz)
+                arr = np.linspace(start.value, end.value, periods)
+                index = cls._simple_new(arr.astype('M8[ns]'), freq=None, tz=tz)


One of two things changed from the existing version. ATM this is:

index = tools.to_datetime(np.linspace(start.value, end.value, periods), utc=True) index = index.tz_convert(tz)

Changed to avoid the dependency on tools.

jbrockmendel · 2018-07-21T21:06:07Z

pandas/core/arrays/datetimes.py

+        dates = list(xdr)
+        # utc = len(dates) > 0 and dates[0].tzinfo is not None
+        values = np.array([x.value for x in dates])
+        data = cls._simple_new(values, freq=freq, tz=tz)


This is the second change from the existing version. ATM this reads:

# utc = len(dates) > 0 and dates[0].tzinfo is not None data = tools.to_datetime(dates)

Maybe you can go ahead and blow away the commented line of code above and just directly use xdr:

values = np.array([x.value for x in xdr])

Good idea, will do. I'd like to track down the source of the comment and make sure it isn't something important.

hmm this list conversion is problematic

codecov · 2018-07-21T22:12:17Z

Codecov Report

Merging #22016 into master will increase coverage by <.01%.
The diff coverage is 94.64%.

@@            Coverage Diff             @@
##           master   #22016      +/-   ##
==========================================
+ Coverage   92.02%   92.03%   +<.01%     
==========================================
  Files         170      170              
  Lines       50707    50743      +36     
==========================================
+ Hits        46661    46699      +38     
+ Misses       4046     4044       -2

Flag	Coverage Δ
#multiple	`90.44% <94.64%> (+0.01%)`	⬆️
#single	`42.35% <56.85%> (+0.03%)`	⬆️

Impacted Files	Coverage Δ
pandas/core/indexes/timedeltas.py	`91.16% <100%> (-0.23%)`	⬇️
pandas/core/indexes/base.py	`96.37% <100%> (ø)`	⬆️
pandas/core/arrays/period.py	`91.22% <100%> (+0.03%)`	⬆️
pandas/core/indexes/datetimes.py	`95.68% <100%> (+0.55%)`	⬆️
pandas/core/indexes/datetimelike.py	`97.77% <100%> (+0.42%)`	⬆️
pandas/core/arrays/datetimes.py	`95.46% <91.2%> (-0.85%)`	⬇️
pandas/core/arrays/timedeltas.py	`92.53% <95%> (+0.77%)`	⬆️
pandas/core/arrays/datetimelike.py	`95.06% <96.46%> (+0.57%)`	⬆️

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 a6c7387...e33ff43. Read the comment docs.

jreback · 2018-07-23T11:06:43Z

pandas/core/arrays/datetimelike.py

+        """
+        if is_period_dtype(cls):
+            # Frequency validation is not meaningful for Period Array/Index
+            return


be explicit about the return

jreback · 2018-07-23T11:08:31Z

pandas/core/arrays/datetimelike.py

+                raise TypeError("cannot add {dtype}-dtype to {cls}"
+                                .format(dtype=other.dtype,
+                                        cls=type(self).__name__))
+            elif is_categorical_dtype(other):


better is is_extension_array_dtype

jreback · 2018-07-23T11:08:51Z

pandas/core/arrays/datetimelike.py

+                raise TypeError("cannot subtract {dtype}-dtype from {cls}"
+                                .format(dtype=other.dtype,
+                                        cls=type(self).__name__))
+            elif is_categorical_dtype(other):


jreback · 2018-07-23T11:13:00Z

pandas/core/arrays/datetimes.py

+        dates = list(xdr)
+        # utc = len(dates) > 0 and dates[0].tzinfo is not None
+        values = np.array([x.value for x in dates])
+        data = cls._simple_new(values, freq=freq, tz=tz)


hmm this list conversion is problematic

jreback · 2018-07-23T11:13:15Z

pandas/core/arrays/datetimes.py

+
+
+def _infer_tz_from_endpoints(start, end, tz):
+    try:


for sure add doc-strings in future

jbrockmendel · 2018-07-23T14:18:42Z

hmm this list conversion is problematic

Why?

…cache2

jreback · 2018-07-24T00:12:19Z

hmm this list conversion is problematic

you are turning an array into a list, only to turn it back to an array

jbrockmendel · 2018-07-24T00:16:25Z

you are turning an array into a list, only to turn it back to an array

It's the same in the existing implementation; we can open an issue if there's a real perf impact.

…cache2

jreback · 2018-07-24T22:12:57Z

it's the same in the existing implementation; we can open an issue if there's a real perf impact.

ok if that's true, then this is prob an overblow concern. pls give a quick test to see if this is actually an issue.

rebase in an y event.

…cache2

jorisvandenbossche · 2018-07-26T08:23:22Z

pandas/core/indexes/datetimelike.py

@@ -754,44 +593,6 @@ def isin(self, values):

        return algorithms.isin(self.asi8, values.asi8)

-    def shift(self, n, freq=None):


Should we keep a stub calling the super method here as well? (as you did with the arithmetic methods or with _generate_range)
I know it is not strictly needed here, but I have the feeling that will make it more clear that then in the future we need to update this from calling super class method to calling the composited values method (although I suppose we have tests for it that will also be a reminder to do it)

For _generate_range and a few others it is needed because the Index method needs to set result.name.

There are a handful of other things things in the same situation as shift (e.g. several properties that are overriden by cache_readonlys) that will need to be edited if/when the change from subclassing to composition is made. I'll make a note not to forget shift when the time comes.

jreback · 2018-07-26T12:49:52Z

thanks @jbrockmendel, pls follow up with @jorisvandenbossche comments in next PR

jbrockmendel · 2018-07-26T20:07:21Z

@TomAugspurger I think with this PeriodArrayMixin has everything needed for comparisons+arithmetic, so PeriodBlock should be viable.

jorisvandenbossche · 2018-07-26T20:21:00Z

@jbrockmendel you are planning to do a PR for that?

jbrockmendel · 2018-07-26T20:55:11Z

you are planning to do a PR for that?

IIRC Tom wanted to be informed when the arith/compare parts were ready so he could forge ahead with PeriodBlock.

My current focus is on fixing DataFrame/Series ops inconsistencies by having DataFrame dispatch to Series (#22068, #22019) and getting thorough tests for these new EA subclasses (#22033)

TomAugspurger · 2018-07-26T21:08:34Z

Thanks. I'm hoping to knock out SparseArray this week or, so if anyone else wants to do Period then by all means. Otherwise I'll start on it after sparse.

…

On Thu, Jul 26, 2018 at 3:55 PM jbrockmendel ***@***.***> wrote: you are planning to do a PR for that? IIRC Tom wanted to be informed when the arith/compare parts were ready so he could forge ahead with PeriodBlock. My current focus is on fixing DataFrame/Series ops inconsistencies by having DataFrame dispatch to Series (#22068 <#22068>, #22019 <#22019>) and getting thorough tests for these new EA subclasses (#22033 <#22033>) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#22016 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQHIsw1L1ZtLtEcms0BN-7aHudIiGqrks5uKiy0gaJpZM4VZwvc> .

jorisvandenbossche · 2018-07-26T21:13:14Z

@jbrockmendel since you are now very familiar with code, I would say, certainly go ahead (if you have time of course)

…22016)

move range-generation functions

c2d70c8

jbrockmendel commented Jul 21, 2018

View reviewed changes

flake8 fixup

5b0ad23

jbrockmendel commented Jul 21, 2018

View reviewed changes

gfyoung added Refactor Internal refactoring of code ExtensionArray Extending pandas with custom dtypes or arrays. labels Jul 23, 2018

jreback requested changes Jul 23, 2018

View reviewed changes

jbrockmendel added 2 commits July 23, 2018 07:19

Merge branch 'master' of https://github.com/pandas-dev/pandas into no…

aa36838

…cache2

docstring, check extension_dtype instead of categorical_dtype

8f45573

Merge branch 'master' of https://github.com/pandas-dev/pandas into no…

2293a72

…cache2

jreback added this to the 0.24.0 milestone Jul 24, 2018

jbrockmendel added 3 commits July 24, 2018 17:47

Merge branch 'master' of https://github.com/pandas-dev/pandas into no…

140255d

…cache2

dummy commit to force CI

9e79704

Merge branch 'master' of https://github.com/pandas-dev/pandas into no…

e33ff43

…cache2

jorisvandenbossche approved these changes Jul 26, 2018

View reviewed changes

jorisvandenbossche changed the title ~~move range-generation functions to EA mixin classes~~ REF: move range-generation functions to EA mixin classes Jul 26, 2018

jreback approved these changes Jul 26, 2018

View reviewed changes

jreback merged commit a1a2f66 into pandas-dev:master Jul 26, 2018

jbrockmendel deleted the nocache2 branch July 26, 2018 16:22

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

REF: move range-generation functions to EA mixin classes (pandas-dev#…

1ce5972

…22016)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

REF: move range-generation functions to EA mixin classes #22016

REF: move range-generation functions to EA mixin classes #22016

jbrockmendel commented Jul 21, 2018

pep8speaks commented Jul 21, 2018 •

edited

Loading

jbrockmendel Jul 21, 2018

jbrockmendel Jul 21, 2018

mroeschke Jul 22, 2018

jbrockmendel Jul 23, 2018

jreback Jul 23, 2018

codecov bot commented Jul 21, 2018 •

edited

Loading

jreback Jul 23, 2018

jreback Jul 23, 2018

jreback Jul 23, 2018

jreback Jul 23, 2018

jreback Jul 23, 2018

jbrockmendel commented Jul 23, 2018

jreback commented Jul 24, 2018

jbrockmendel commented Jul 24, 2018

jreback commented Jul 24, 2018

jorisvandenbossche Jul 26, 2018

jbrockmendel Jul 26, 2018

jreback commented Jul 26, 2018

jbrockmendel commented Jul 26, 2018

jorisvandenbossche commented Jul 26, 2018

jbrockmendel commented Jul 26, 2018

TomAugspurger commented Jul 26, 2018 via email

jorisvandenbossche commented Jul 26, 2018

		@@ -754,44 +593,6 @@ def isin(self, values):

		return algorithms.isin(self.asi8, values.asi8)

		def shift(self, n, freq=None):

REF: move range-generation functions to EA mixin classes #22016

REF: move range-generation functions to EA mixin classes #22016

Conversation

jbrockmendel commented Jul 21, 2018

pep8speaks commented Jul 21, 2018 • edited Loading

Comment last updated on July 25, 2018 at 19:12 Hours UTC

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 21, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Jul 23, 2018

jreback commented Jul 24, 2018

jbrockmendel commented Jul 24, 2018

jreback commented Jul 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Jul 26, 2018

jbrockmendel commented Jul 26, 2018

jorisvandenbossche commented Jul 26, 2018

jbrockmendel commented Jul 26, 2018

TomAugspurger commented Jul 26, 2018 via email

jorisvandenbossche commented Jul 26, 2018

pep8speaks commented Jul 21, 2018 •

edited

Loading

codecov bot commented Jul 21, 2018 •

edited

Loading