REF: Simplify Period/Datetime Array/Index constructors #23093

jbrockmendel · 2018-10-11T15:19:19Z

Split off from the same branch that spawned #23083.

pep8speaks · 2018-10-11T15:19:31Z

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/arrays/period.py !
There are no PEP8 issues in the file pandas/core/arrays/timedeltas.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/indexes/period/test_construction.py !

jbrockmendel · 2018-10-11T15:19:58Z

pandas/core/arrays/datetimes.py

+        if freq is None and any(x is None for x in [periods, start, end]):
+            raise ValueError('Must provide freq argument if no data is '
+                             'supplied')
+


Moved these checks here from inside __new__

+1, as long as we're OK with ignoring gibberish arguments when there not actually used (which seems to be what we do right now).

In [9]: idx = pd.PeriodIndex(['2000', '2001'], freq='D') In [10]: pd.PeriodIndex(idx, start='foo') Out[10]: PeriodIndex(['2000-01-01', '2001-01-01'], dtype='period[D]', freq='D')

jbrockmendel · 2018-10-11T15:22:24Z

pandas/core/arrays/period.py


    @classmethod
-    def _from_ordinals(cls, values, freq=None):
+    def _from_ordinals(cls, values, freq=None, **kwargs):


Ideally I'd like to get rid of _from_ordinals and have _simple_new be the lowest-level constructor like with all the other classes. This turns out to be infeasible ATM because a bunch of parent class methods operate on .values instead of ._ndarray_values and end up passing object-dtype to _shallow_copy. This is being split off of a branch that is trying to avoid that.

jbrockmendel · 2018-10-11T15:23:04Z

pandas/core/arrays/timedeltas.py

-            periods = dtl.validate_periods(periods)
-            return cls._generate_range(start, end, periods, freq,
-                                       closed=closed)
-


This is still present in the TimedeltaIndex constructor, but its not too late to get it out of the TimedeltaArray constructor.

jorisvandenbossche · 2018-10-11T15:34:20Z

pandas/core/arrays/period.py


-        return cls._from_ordinals(values, freq)
+        return cls._from_ordinals(values, freq=freq, **kwargs)


was this causing bugs that kwargs was not passed?

This is necessary in order to allow PeriodIndex to inherit _simple_new from PeriodArrayMixin.

The PeriodIndex constructors in particular (but also DatetimeIndex and TimedeltaIndex) are really messy and complicated. Inheriting methods and deleting as much as possible makes it easier for me to simplify them, even if some of that will end up getting copy/pasted down the road if/when inheritance is replaced.

Can you then move the implementation and share it by calling the other class instead of inheriting?

Do you mean moving the _from_ordinals implementation? We need both because the Index version sets self.name and calls _reset_identity

I meant in PeriodIndex._simple_new calling PeriodArrayMixin._simple_new, similar as you did in one of the previous PRs (didn't look at the code, so don't know if that is possible in this case)

I think I get what you're saying, will give it a shot

Did anything come of this, one way or the other?

Yes. In an earlier commit I deleted PeriodIndex._simple_new and retained PeriodIndex._from_ordinals. Now that has been reversed, and its non-trivially nicer.

jorisvandenbossche · 2018-10-11T15:35:27Z

pandas/core/arrays/period.py

        """
        Values should be int ordinals
        `__new__` & `_simple_new` cooerce to ordinals and call this method
        """
+        # **kwargs are included so that the signature matches PeriodIndex,
+        #  letting us share _simple_new


Why do you want to share this? Once we actually don't inherit it anymore, we will need to add it back to PeriodIndex

I think the signature should match across index classes, and hopefully the implementation can be shared.

But this one is about _from_ordinals, which will not be shared across index classes as it is Period-specific

I think we're moving towards getting rid of _from_ordinals in order to make PeriodIndex work like all the others. To do that, we need to simplify _simple_new, which means fixing the places where inappropriate values get passed to it, i.e. #23095.

jorisvandenbossche · 2018-10-11T15:41:27Z

pandas/core/indexes/period.py

-                raise TypeError("PeriodIndex can't take floats")
-            return cls(values, name=name, freq=freq, **kwargs)
-
-        return cls._from_ordinals(values, name, freq, **kwargs)


If keeping this now, I think it will be clearer what the changes are once we actually split index/array

I don't think it matters too much either way. @jbrockmendel does reverting this change cause issues with master? Or does the "old" PeriodIndex._simple_new do the right thing, at the cost of code duplication? My PeriodArray PR is going to change this again anyway

It has changed since joris's comment above, but this is just a matter of code duplication.

My PeriodArray PR is going to change this again anyway

I'm kind of hoping I can simplify these a good deal further before you finish up with SparseEA

jbrockmendel · 2018-10-11T18:00:23Z

pandas/io/pytables.py

                                                   freq=freq)
                if tz is not None:
                    result = result.tz_localize('UTC').tz_convert(tz)
                return result
            return f
        elif klass == PeriodIndex:
            def f(values, freq=None, tz=None):
-                return PeriodIndex._simple_new(values, None, freq=freq)
+                return PeriodIndex._simple_new(values, name=None, freq=freq)


Really these shouldn't be calling _simple_new at all.

well, this is actually tricky. the period codes are actually stored. We in fact need to think about this for EA, potentially providing an easy way to have serialization code deal with this.

Why isn't this from_ordinals?

Turns out changing this to just call PeriodIndex(...) causes some test failures. Tried it in #23140.

jorisvandenbossche · 2018-10-11T18:00:41Z

Also, to what extent is not conflicting with the work @TomAugspurger is doing in the PeriodArray PR?

TomAugspurger · 2018-10-11T18:04:23Z

I haven't looked through this yet, but the constructors are a major pain point on that PR right now.

…

On Thu, Oct 11, 2018 at 1:00 PM Joris Van den Bossche < ***@***.***> wrote: Also, to what extent is not conflicting with the work @TomAugspurger <https://github.com/TomAugspurger> is doing in the PeriodArray PR? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23093 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQHIvUHbNjhgrix0AH8hvpdLlyX6KWQks5uj4dTgaJpZM4XXx8L> .

jorisvandenbossche · 2018-10-11T18:06:04Z

But the question is then maybe: do we want to solve that before or after that PR (or in) ?

codecov · 2018-10-11T20:08:45Z

Codecov Report

Merging #23093 into master will increase coverage by 0.01%.
The diff coverage is 93.54%.

@@            Coverage Diff             @@
##           master   #23093      +/-   ##
==========================================
+ Coverage    92.2%   92.21%   +0.01%     
==========================================
  Files         169      169              
  Lines       50924    50920       -4     
==========================================
+ Hits        46952    46955       +3     
+ Misses       3972     3965       -7

Flag	Coverage Δ
#multiple	`90.63% <90.32%> (+0.01%)`	⬆️
#single	`42.3% <58.06%> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/core/indexes/datetimes.py	`95.72% <ø> (-0.02%)`	⬇️
pandas/core/arrays/period.py	`94.29% <100%> (+1.43%)`	⬆️
pandas/core/indexes/period.py	`93.43% <100%> (-0.02%)`	⬇️
pandas/io/pytables.py	`92.44% <100%> (ø)`	⬆️
pandas/core/arrays/datetimes.py	`96.94% <100%> (+0.02%)`	⬆️
pandas/core/arrays/timedeltas.py	`93.96% <66.66%> (+1.43%)`	⬆️
pandas/core/arrays/datetimelike.py	`95.41% <90%> (-0.15%)`	⬇️

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 c8ce3d0...04c75ca. Read the comment docs.

TomAugspurger

Before we go too far down this path, can we come up with what we would like out
of our Index constructors?

Some _simple_news accept and use a dtype. Others, like CategoricalIndex,
let you override the ordered or categories. I'd like to get away from that if
possible. I'd rather put the responsibility of getting the underlying data in
the right shape on the caller.

Ideally, I would like _simple_new to have the signature
(Union[ndarray, ExtensionArray], name).
All _attributes that we previously carried around on the instance would go on
the Array. This would make _simple_new truly simple:

@classmethod
def _simple_new(cls, values, name=None):
    # type: (Union[ndarray, ExtensionArray], Optional[Any]) -> Index
    result = object.__new__(cls)
    result._data = values
    result.name = name
    result._reset_identity()
    return result

I haven't investigated whether this is feasible (it's certainly blocked by
DatetimeArray, since we need somewhere to put the .tz).

Closely related, what do we want out of _shallow_copy and
_shallow_copy_with_infer? In what ways do they differ from a
return self._simple_new(self.values, name=self.name)? Do they need to accept a
values argument?

TomAugspurger · 2018-10-11T21:05:18Z

pandas/core/arrays/datetimes.py

+        if freq is None and any(x is None for x in [periods, start, end]):
+            raise ValueError('Must provide freq argument if no data is '
+                             'supplied')
+


+1, as long as we're OK with ignoring gibberish arguments when there not actually used (which seems to be what we do right now).

In [9]: idx = pd.PeriodIndex(['2000', '2001'], freq='D') In [10]: pd.PeriodIndex(idx, start='foo') Out[10]: PeriodIndex(['2000-01-01', '2001-01-01'], dtype='period[D]', freq='D')

jorisvandenbossche · 2018-10-11T21:33:47Z

Fully agree on the proposed _simple_new interface.

Closely related, what do we want out of _shallow_copy and
_shallow_copy_with_infer? In what ways do they differ from a
return self._simple_new(self.values, name=self.name)? Do they need to accept a
values argument?

We have been discussing those in #22961 as well

jbrockmendel · 2018-10-11T22:20:16Z

All _attributes that we previously carried around on the instance would go on the Array. This would make _simple_new truly simple

I like this idea.

pd.PeriodIndex(idx, start='foo')

Given that we have an opportunity to start fresh with the EA subclasses, I think we should avoid letting this happen there. In the Index subclasses it would be nice to fix, but can be considered separately.

Closely related, what do we want out of _shallow_copy and
_shallow_copy_with_infer?

Part of my takeaway from #23095 is that moving away from both of these would be helpful medium-term.

jbrockmendel · 2018-10-11T22:38:09Z

I'm hopeful that we're close to a consensus here and in #23095 (and tangentially related #23031). If not, is there a non-controversial subset of this that can be broken off? These are blockers.

TomAugspurger

Only question is on PeriodIndex._simple_new, but it doesn't matter either way. I'm fine with whatever gets us to our goal with the least amount of energy.

TomAugspurger · 2018-10-12T01:43:05Z

pandas/core/indexes/period.py

-                raise TypeError("PeriodIndex can't take floats")
-            return cls(values, name=name, freq=freq, **kwargs)
-
-        return cls._from_ordinals(values, name, freq, **kwargs)


I don't think it matters too much either way. @jbrockmendel does reverting this change cause issues with master? Or does the "old" PeriodIndex._simple_new do the right thing, at the cost of code duplication? My PeriodArray PR is going to change this again anyway

TomAugspurger · 2018-10-12T01:43:31Z

pandas/io/pytables.py

                                                   freq=freq)
                if tz is not None:
                    result = result.tz_localize('UTC').tz_convert(tz)
                return result
            return f
        elif klass == PeriodIndex:
            def f(values, freq=None, tz=None):
-                return PeriodIndex._simple_new(values, None, freq=freq)
+                return PeriodIndex._simple_new(values, name=None, freq=freq)


jreback · 2018-10-12T12:45:17Z

pandas/io/pytables.py

                                                   freq=freq)
                if tz is not None:
                    result = result.tz_localize('UTC').tz_convert(tz)
                return result
            return f
        elif klass == PeriodIndex:
            def f(values, freq=None, tz=None):
-                return PeriodIndex._simple_new(values, None, freq=freq)
+                return PeriodIndex._simple_new(values, name=None, freq=freq)


well, this is actually tricky. the period codes are actually stored. We in fact need to think about this for EA, potentially providing an easy way to have serialization code deal with this.

Why isn't this from_ordinals?

jreback · 2018-10-12T12:45:50Z

note there is a bug out there about deserialization of HDF5 Period types. This may solve it

jbrockmendel · 2018-10-12T15:45:48Z

Why isn't this from_ordinals?

No idea what was originally intended; I want to go the other way with it and use a public constructor (i.e. __new__)

jreback · 2018-10-12T21:50:39Z

ok this looks fine.

)

Simplify Period/Datetime Array/Index constructors

0e40ed4

jbrockmendel commented Oct 11, 2018

View reviewed changes

revert removal of kwargs

dc63072

jorisvandenbossche reviewed Oct 11, 2018

View reviewed changes

fix _simple_new calls in pytables

ee210e4

jbrockmendel commented Oct 11, 2018

View reviewed changes

jbrockmendel mentioned this pull request Oct 11, 2018

PERF: PeriodIndex.dropna, difference, take #23095

Closed

jbrockmendel added 2 commits October 11, 2018 11:39

Fixup unused import

fc88e59

override simple_new instead of _from_ordinasl

69a8f7e

TomAugspurger reviewed Oct 11, 2018

View reviewed changes

Call simple_new instead of from_ordinals

04c75ca

TomAugspurger approved these changes Oct 12, 2018

View reviewed changes

jreback requested changes Oct 12, 2018

View reviewed changes

jreback added Period Period data type Clean labels Oct 12, 2018

jreback added this to the 0.24.0 milestone Oct 12, 2018

jreback approved these changes Oct 12, 2018

View reviewed changes

jreback merged commit e4b67ca into pandas-dev:master Oct 12, 2018

jbrockmendel deleted the pi_cons branch October 12, 2018 22:15

jorisvandenbossche mentioned this pull request Oct 17, 2018

API: Index and Array constructors design #23212

Closed

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

REF: Simplify Period/Datetime Array/Index constructors (pandas-dev#23093

93178d3

)


		return cls._from_ordinals(values, freq)
		return cls._from_ordinals(values, freq=freq, **kwargs)

REF: Simplify Period/Datetime Array/Index constructors #23093

REF: Simplify Period/Datetime Array/Index constructors #23093

Conversation

jbrockmendel commented Oct 11, 2018

pep8speaks commented Oct 11, 2018

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

Choose a reason for hiding this comment

TomAugspurger Oct 12, 2018 • edited Loading

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

jorisvandenbossche commented Oct 11, 2018

TomAugspurger commented Oct 11, 2018 via email

jorisvandenbossche commented Oct 11, 2018

codecov bot commented Oct 11, 2018 • edited Loading

Codecov Report

TomAugspurger left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Oct 11, 2018

jbrockmendel commented Oct 11, 2018

jbrockmendel commented Oct 11, 2018

TomAugspurger 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

jreback commented Oct 12, 2018 • edited Loading

jbrockmendel commented Oct 12, 2018

jreback commented Oct 12, 2018

TomAugspurger Oct 12, 2018 •

edited

Loading

codecov bot commented Oct 11, 2018 •

edited

Loading

TomAugspurger left a comment •

edited

Loading

jreback commented Oct 12, 2018 •

edited

Loading