CLN: Move to_period, to_perioddelta up to EA subclasses #23113

jbrockmendel · 2018-10-12T16:14:28Z

Should be orthogonal to other outstanding datetimelike EA PRs.

…ike6

pep8speaks · 2018-10-12T16:14:32Z

Hello @jbrockmendel! Thanks for submitting the PR.

There are no PEP8 issues in the file pandas/core/arrays/datetimelike.py !
There are no PEP8 issues in the file pandas/core/arrays/datetimes.py !
There are no PEP8 issues in the file pandas/core/indexes/datetimelike.py !
There are no PEP8 issues in the file pandas/core/indexes/datetimes.py !
There are no PEP8 issues in the file pandas/core/indexes/period.py !
There are no PEP8 issues in the file pandas/tests/arrays/test_datetimelike.py !

codecov · 2018-10-12T22:35:05Z

Codecov Report

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

@@            Coverage Diff             @@
##           master   #23113      +/-   ##
==========================================
- Coverage   92.19%   92.19%   -0.01%     
==========================================
  Files         169      169              
  Lines       50950    50948       -2     
==========================================
- Hits        46975    46973       -2     
  Misses       3975     3975

Flag	Coverage Δ
#multiple	`90.61% <100%> (-0.01%)`	⬇️
#single	`42.27% <33.33%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/indexes/period.py	`93.24% <100%> (ø)`	⬆️
pandas/core/indexes/datetimes.py	`96.46% <100%> (-0.04%)`	⬇️
pandas/core/arrays/datetimes.py	`97.4% <100%> (+0.02%)`	⬆️

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 b54e20b...5d8bc1a. Read the comment docs.

jorisvandenbossche

to_period is fine to move for me, but for the others we need to think a bit about it

To what extent do we want to keep our internal EAs to be strict to the EA interface (except for additional type-specific methods like to_period of course)?
I think we should try to be strict, as otherwise we might start relying on those extra methods, that might not be there on external EAs.

But we of course also discuss to expand the EA interface.

jorisvandenbossche · 2018-10-13T08:43:05Z

pandas/core/arrays/datetimelike.py

@@ -207,6 +208,24 @@ def astype(self, dtype, copy=True):
            return self._box_values(self.asi8)
        return super(DatetimeLikeArrayMixin, self).astype(dtype, copy)

+    def repeat(self, repeats, *args, **kwargs):


Is a repeat method needed? For now, we don't have that in our EA interface, so if avoidable, I would keep it that way (and keep it on the index).

repeat is also moved to PeriodArray in #22862.

So we might need to discuss it there as well.

But also, in that PR, the index method still exists. So in this PR, you are removing something from the index, which will need to be added back in a next PR.

jorisvandenbossche · 2018-10-13T08:47:02Z

pandas/core/arrays/datetimes.py

+        """
+        Calculate TimedeltaArray of difference between index
+        values and index converted to PeriodArray at specified
+        freq. Used for vectorized offsets


I find this a rather "obscure" method (meaning, not directly sure how it would be useful for me as a user). But I am also not a frequent user of periods/offsets, so difficult to say (and maybe a good example in the docstring might already help).

So what I want to say: in case this is indeed rather for internal use, I would not make it a public method on the array class (but can still do the move but make it for example private here)

This is used indirectly in DatetimeIndex + DateOffset, so will be needed for DatetimeArray + DateOffset.

we might want to make this private, i dont' think we need to expose to the user

I privatized this earlier today, but have now changed my mind. For DateOffset arithmetic to work "for free" it needs to have the same name. We can revisit this later if needbe.

OK, can you then add a comment like TODO(EA): make this private, for once we do the actual split (because I suppose then this is no longer a problem?)

jorisvandenbossche · 2018-10-13T08:50:26Z

pandas/core/arrays/datetimelike.py

+        return self._shallow_copy(self.asi8.repeat(repeats),
+                                  freq=freq)
+
+    def tolist(self):


same for tolist

this is ok, again a common / useful method. actually I would move these up to array.base (and if needed subclass here for example repeat)

But then let's first have a general discussion about adding it to the EA interface, and what the exact expected semantics are.

But unless we have a clear use case for pandas (and have we? or is this on the Index rather for users?), I think we should a bit hesitant to add a lot.

(and also the same comment applies here: even if we add it to the Array object, it should not be removed from the Index)

jbrockmendel · 2018-10-13T16:37:15Z

To what extent do we want to keep our internal EAs to be strict to the EA interface

I've been thinking about it from the other direction: what methods are Index-specific versus making sense on an array?

But we of course also discuss to expand the EA interface.

There is a growing list of Issues of the form "Add X to EA" that has me convinced we're going to end up with basically the entire ndarray API. Which makes sense to the extent we want EA (or at least pandas-internal EAs) to be a seamless drop-in replacement.

…ike6

jreback · 2018-10-14T13:25:04Z

There is a growing list of Issues of the form "Add X to EA" that has me convinced we're going to end up with basically the entire ndarray API. Which makes sense to the extent we want EA (or at least pandas-internal EAs) to be a seamless drop-in replacement.

I am with @jbrockmendel here. The EA interface is going be used as a drop in replacement for ndarray things, so we ought to have the common / useful methods at least. and certainly .repeat is one of those.

jreback · 2018-10-14T13:26:06Z

pandas/core/arrays/datetimelike.py

+        return self._shallow_copy(self.asi8.repeat(repeats),
+                                  freq=freq)
+
+    def tolist(self):


this is ok, again a common / useful method. actually I would move these up to array.base (and if needed subclass here for example repeat)

jreback · 2018-10-14T13:26:38Z

pandas/core/arrays/datetimes.py

+        """
+        Calculate TimedeltaArray of difference between index
+        values and index converted to PeriodArray at specified
+        freq. Used for vectorized offsets


we might want to make this private, i dont' think we need to expose to the user

jbrockmendel · 2018-10-14T15:26:50Z

we might want to make this private, i dont' think we need to expose to the user

OK. We can make it private on the Array and keep it public on the Index (could privatize later if really wanted)

…ike6

jorisvandenbossche · 2018-10-14T20:13:06Z

pandas/core/arrays/datetimelike.py

@@ -207,6 +208,24 @@ def astype(self, dtype, copy=True):
            return self._box_values(self.asi8)
        return super(DatetimeLikeArrayMixin, self).astype(dtype, copy)

+    def repeat(self, repeats, *args, **kwargs):


So we might need to discuss it there as well.

But also, in that PR, the index method still exists. So in this PR, you are removing something from the index, which will need to be added back in a next PR.

jorisvandenbossche · 2018-10-14T20:14:44Z

pandas/core/arrays/datetimelike.py

+        return self._shallow_copy(self.asi8.repeat(repeats),
+                                  freq=freq)
+
+    def tolist(self):


But then let's first have a general discussion about adding it to the EA interface, and what the exact expected semantics are.

But unless we have a clear use case for pandas (and have we? or is this on the Index rather for users?), I think we should a bit hesitant to add a lot.

(and also the same comment applies here: even if we add it to the Array object, it should not be removed from the Index)

jbrockmendel · 2018-10-14T22:28:43Z

Reverted move of repeat/tolist so we can move the ball down the field.

…ike6

jreback · 2018-10-15T12:36:26Z

lgtm. The repeat/tolist were fine actually.

jorisvandenbossche · 2018-10-15T12:42:37Z

I have still one (very minor) outstanding comment: https://github.com/pandas-dev/pandas/pull/23113/files#r225054241, then good to merge for me as well

…dlike6

jbrockmendel · 2018-10-15T14:57:21Z

comment added

jbrockmendel · 2018-10-15T19:02:59Z

@jorisvandenbossche implemented the requested comment. Are there other outstanding issues here?

…ike6

TomAugspurger

Looks good. I don't think this should cause issues.

I restarted the travis job that timed out.

jbrockmendel · 2018-10-18T02:01:15Z

@jorisvandenbossche I think this is as green as it gets at the moment. Would like to explicitly get your OK.

…ike6

jorisvandenbossche · 2018-10-18T20:31:10Z

Also the import order that is failing

…ike6

jorisvandenbossche · 2018-10-22T09:42:32Z

Thanks!

jorisvandenbossche · 2018-10-22T09:43:17Z

Thanks!

…3113)

TomAugspurger · 2018-12-10T16:34:43Z

pandas/core/indexes/datetimes.py

@@ -1168,6 +1143,9 @@ def slice_indexer(self, start=None, end=None, step=None, kind=None):
    is_year_end = wrap_field_accessor(DatetimeArrayMixin.is_year_end)
    is_leap_year = wrap_field_accessor(DatetimeArrayMixin.is_leap_year)

+    to_perioddelta = wrap_array_method(DatetimeArrayMixin.to_perioddelta,
+                                       False)


Does anyone know why the result's .name isn't set to self.name here?

jbrockmendel added 7 commits October 11, 2018 08:43

Move repeat up to array mixin

e2c6727

Move tolist up

5c32348

Move to_perioddelta up

ba5c2c1

Avoid passing object dtype to simple_new

e1e05a9

de-duplicate wrapping code

d3ca5e9

de-duplicate wrapping code

b4c2496

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

192170b

…ike6

jbrockmendel added 2 commits October 12, 2018 11:25

test fixup

b8c2647

Fixup unused import

a99b1f1

jorisvandenbossche reviewed Oct 13, 2018

View reviewed changes

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

edea5ef

…ike6

jreback requested changes Oct 14, 2018

View reviewed changes

jreback added Datetime Datetime data dtype Period Period data type ExtensionArray Extending pandas with custom dtypes or arrays. labels Oct 14, 2018

jbrockmendel added 2 commits October 14, 2018 08:27

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

a4ee53d

…ike6

privatize to_perioddelta in Array class

eaf364f

jorisvandenbossche requested changes Oct 14, 2018

View reviewed changes

jbrockmendel added 2 commits October 14, 2018 15:27

revert move of repeat, tolist

0a5bb77

remove tests for removed methods

b38ad9b

jbrockmendel added 2 commits October 14, 2018 16:55

Revert privatization in test

28bcd5a

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

033960a

…ike6

jreback added this to the 0.24.0 milestone Oct 15, 2018

jreback approved these changes Oct 15, 2018

View reviewed changes

jbrockmendel added 2 commits October 15, 2018 07:56

add comment [ci skip]

91aaef4

Merge branch 'dlike6' of https://github.com/jbrockmendel/pandas into …

44c8162

…dlike6

jorisvandenbossche changed the title ~~Move to_period, to_perioddelta, repeat up to EA subclasses~~ CLN: Move to_period, to_perioddelta up to EA subclasses Oct 15, 2018

TomAugspurger mentioned this pull request Oct 16, 2018

Datetimelike Array Refactor #23185

Closed

jbrockmendel added 2 commits October 16, 2018 17:41

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

ef5c1e6

…ike6

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

3b31903

…ike6

TomAugspurger approved these changes Oct 17, 2018

View reviewed changes

jbrockmendel mentioned this pull request Oct 18, 2018

Use align_method in comp_method_FRAME #23132

Merged

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

0ce415b

…ike6

jorisvandenbossche approved these changes Oct 18, 2018

View reviewed changes

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

5d8bc1a

…ike6

jbrockmendel mentioned this pull request Oct 20, 2018

REF: Make PeriodArray an ExtensionArray #22862

Merged

jorisvandenbossche merged commit 4cac923 into pandas-dev:master Oct 22, 2018

jbrockmendel deleted the dlike6 branch October 22, 2018 15:57

JustinZhengBC pushed a commit to JustinZhengBC/pandas that referenced this pull request Oct 23, 2018

CLN: Move to_period, to_perioddelta up to EA subclasses (pandas-dev#2…

61435fc

…3113)

tm9k1 pushed a commit to tm9k1/pandas that referenced this pull request Nov 19, 2018

CLN: Move to_period, to_perioddelta up to EA subclasses (pandas-dev#2…

3e09d97

…3113)

TomAugspurger reviewed Dec 10, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLN: Move to_period, to_perioddelta up to EA subclasses #23113

CLN: Move to_period, to_perioddelta up to EA subclasses #23113

jbrockmendel commented Oct 12, 2018

pep8speaks commented Oct 12, 2018

codecov bot commented Oct 12, 2018 •

edited

Loading

jorisvandenbossche left a comment

jorisvandenbossche Oct 13, 2018

jbrockmendel Oct 13, 2018

jorisvandenbossche Oct 14, 2018

jorisvandenbossche Oct 13, 2018

jbrockmendel Oct 13, 2018

jreback Oct 14, 2018

jbrockmendel Oct 14, 2018

jorisvandenbossche Oct 15, 2018

jorisvandenbossche Oct 13, 2018

jreback Oct 14, 2018

jorisvandenbossche Oct 14, 2018

jbrockmendel commented Oct 13, 2018

jreback commented Oct 14, 2018

jreback Oct 14, 2018

jreback Oct 14, 2018

jbrockmendel commented Oct 14, 2018

jorisvandenbossche Oct 14, 2018

jorisvandenbossche Oct 14, 2018

jbrockmendel commented Oct 14, 2018

jreback commented Oct 15, 2018

jorisvandenbossche commented Oct 15, 2018 •

edited

Loading

jbrockmendel commented Oct 15, 2018

jbrockmendel commented Oct 15, 2018

TomAugspurger left a comment

jbrockmendel commented Oct 18, 2018

jorisvandenbossche commented Oct 18, 2018

jorisvandenbossche commented Oct 22, 2018

jorisvandenbossche commented Oct 22, 2018

TomAugspurger Dec 10, 2018 •

edited

Loading

CLN: Move to_period, to_perioddelta up to EA subclasses #23113

CLN: Move to_period, to_perioddelta up to EA subclasses #23113

Conversation

jbrockmendel commented Oct 12, 2018

pep8speaks commented Oct 12, 2018

codecov bot commented Oct 12, 2018 • edited Loading

Codecov Report

jorisvandenbossche left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Oct 13, 2018

jreback commented Oct 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Oct 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Oct 14, 2018

jreback commented Oct 15, 2018

jorisvandenbossche commented Oct 15, 2018 • edited Loading

jbrockmendel commented Oct 15, 2018

jbrockmendel commented Oct 15, 2018

TomAugspurger left a comment

Choose a reason for hiding this comment

jbrockmendel commented Oct 18, 2018

jorisvandenbossche commented Oct 18, 2018

jorisvandenbossche commented Oct 22, 2018

jorisvandenbossche commented Oct 22, 2018

TomAugspurger Dec 10, 2018 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Oct 12, 2018 •

edited

Loading

jorisvandenbossche commented Oct 15, 2018 •

edited

Loading

TomAugspurger Dec 10, 2018 •

edited

Loading