8000 REF: Simplify Datetimelike constructor dispatching by jbrockmendel · Pull Request #23140 · pandas-dev/pandas · GitHub
[go: up one dir, main page]
Skip to content

REF: Simplify Datetimelike constructor dispatching #23140

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.

Sign up for GitHub

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

Closed
wants to merge 22 commits into from
Closed

REF: Simplify Datetimelike constructor dispatching #23140

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
f13cc58
Avoid non-public constructors
jbrockmendel Oct 13, 2018
4188ec7
simplify and de-duplicate _generate_range
jbrockmendel Oct 13, 2018
7804f1b
Check for invalid axis kwarg
jbrockmendel Oct 13, 2018
a4775f4
Move some EA properties up to mixins
jbrockmendel Oct 13, 2018
8ee34fa
implement basic TimedeltaArray tests
jbrockmendel Oct 13, 2018
78943c1
clean up PeriodArray constructor, with tests
jbrockmendel Oct 13, 2018
aa71383
make PeriodArray.__new__ more grown-up
jbrockmendel Oct 13, 2018
eae8389
Remove unused kwargs from TimedeltaArray.__new__
jbrockmendel Oct 13, 2018
e871733
revert change that broke tests
jbrockmendel Oct 13, 2018
7840f91
Fixup whitespace
jbrockmendel Oct 14, 2018
ec50b0b
helper function for axis validation
jbrockmendel Oct 14, 2018
eb7a6b6
suggested clarifications
jbrockmendel Oct 14, 2018
32c6391
Merge branch 'dlike8' of https://github.com/jbrockmendel/pandas into …
jbrockmendel Oct 14, 2018
c903917
Merge branch 'master' of https://github.com/pandas-dev/pandas into dl…
jbrockmendel Oct 14, 2018
b97ec96
move axis validation to nv
jbrockmendel Oct 14, 2018
11db555
Merge branch 'master' of https://github.com/pandas-dev/pandas into dl…
jbrockmendel Oct 14, 2018
147de57
revert some removals
jbrockmendel Oct 14, 2018
7c4d281
Merge branch 'master' of https://github.com/pandas-dev/pandas into dl…
jbrockmendel Oct 15, 2018
b90f421
catch too-negative values
jbrockmendel Oct 15, 2018
dc4f474
Roll validate_minmax_axis into existing validate functions
jbrockmendel Oct 15, 2018
46d5e64
fixup typo
jbrockmendel Oct 15, 2018
b5827c7
Merge branch 'master' of https://github.com/pandas-dev/pandas into dl…
jbrockmendel Oct 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
clean up PeriodArray constructor, with tests
  • Loading branch information
@jbrockmendel
jbrockmendel committed Oct 13, 2018
commit 78943c17e2dcab4f9cf1098c18980d06c9dbfe9e
15 changes: 11 additions & 4 deletions pandas/core/arrays/period.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

from pandas.core.dtypes.common import (
is_integer_dtype, is_float_dtype, is_period_dtype,
is_object_dtype,
is_datetime64_dtype)
from pandas.core.dtypes.dtypes import PeriodDtype
from pandas.core.dtypes.generic import ABCSeries
Expand Down Expand Up @@ -124,15 +125,19 @@ def freq(self, value):
def __new__(cls, values, freq=None, **kwargs):
if is_period_dtype(values):
# PeriodArray, PeriodIndex
if freq is not None and values.freq != freq:
raise IncompatibleFrequency(freq, values.freq)
freq = values.freq
freq = dtl.validate_dtype_freq(values.dtype, freq)
values = values.asi8

elif is_datetime64_dtype(values):
# TODO: what if it has tz?
values = dt64arr_to_periodarr(values, freq)

elif is_object_dtype(values) or isinstance(values, (list, tuple)):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be is_list_like? (for the isinstance check)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is specifically for object dtype (actually, I need to add dtype=object to the np.array call below) since we're calling libperiod.extract_ordinals, which expects object dtype.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

specifically what happens if other non ndarray list likes hit this path? do they need handling?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They do need handling, but we're not there yet. The thought process for implementing these constructors piece-by-piece is

a) The DatetimeIndex/TimedeltaIndex/PeriodIndex constructors are overgrown; let's avoid that in the Array subclasses.
b) Avoid letting the implementations get too far ahead of the tests

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other question: where was this handled previously?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's hard for me to say what's better in the abstract.

From the WIP PeriodArray PR, I found that having to think carefully about what type of data I had forced some clarity in the code. I liked having to explicitly reach for that _from_periods constructor.

Regardless, I think our two goals with the array constructors should be

  1. Maximizing developer happiness (i.e. not users at the moment)
  2. Making it easier to reuse code between Index & Array subclasses

If you think we're likely to end up in a situation where being able to pass an array of objects to the main __init__ will make things easier, then by all means.

Copy link
Contributor
@jreback jreback Oct 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am a bit puzzled why you would handle lists and and ndarray differently (tom and joris); these are clearly doing the same thing and we have a very similar handling for list likes throughout pandas

separating these is a non starter - even having a separate constructor is also not very friendly. pandas does inference on the construction which is one of the big selling points. trying to change this, esp at the micro level is a huge mental disconnect.

if you want to propose something like that pls do it in other issues.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am a bit puzzled why you would handle lists and and ndarray differently (tom and joris)

I don't think we are.

But, my only argument was

From the WIP PeriodArray PR, I found that having to think carefully about what type of data I had forced some clarity in the code. I liked having to explicitly reach for that _from_periods constructor.

If that's not persuasive then I'm not going to argue against handling them in the init.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

having to think carefully

+1

Maximizing developer happiness

+1

Making it easier to reuse code

+1

If you think we're likely to end up in a situation where being able to pass an array of objects to the main

Yes, I think we should be pretty forgiving about what gets accepted into __init__ (for all three of Period/Datetime/Timedelta Arrays). Definitely don't want the start, end, periods currently in the Index subclass constructors. I think by excluding those we'll keep these constructors fairly straightforward.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am a bit puzzled why you would handle lists and and ndarray differently

It's not about lists vs arrays, it's about arrays of Period objects vs arrays of ordinal integers, which is something very different.

I think we should be pretty forgiving about what gets accepted into init

Being forgiving is exactly what lead to the complex Period/DatetimeIndex constructors. I think we should not make the same choice for our Array classes.
Of course it doesn't need to be that complex, as I think there are here EDBE two main usecases discussed: an array of scalar objects (eg Periods or Timestamps), or an array of the underlying storage type (eg datetime64 or ordinal integers).

I personally also think it makes the code clearer to even separate those two concepts (basically what we also did with IntegerArray), but maybe let's open an issue to further discuss that instead of here in a hidden review comment thread? (i can only open one later today )

# e.g. array([Period(...), Period(...), NaT])
values = np.array(values)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if this is an int array? or is that prohibited? (except via _from_ordinals)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then it gets passed through simple_new unchanged.

if freq is None:
freq = libperiod.extract_freq(values)
values = libperiod.extract_ordinals(values, freq)

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

@classmethod
Expand Down Expand Up @@ -175,6 +180,8 @@ def _from_ordinals(cls, values, freq=None, **kwargs):

@classmethod
def _generate_range(cls, start, end, periods, freq, fields):
periods = dtl.validate_periods(periods)

if freq is not None:
freq = Period._maybe_convert_freq(freq)

Expand Down
3 changes: 1 addition & 2 deletions pandas/core/indexes/period.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,8 +165,6 @@ def __new__(cls, data=None, ordinal=None, freq=None, start=None, end=None,
raise TypeError('__new__() got an unexpected keyword argument {}'.
format(list(set(fields) - valid_field_set)[0]))

periods = dtl.validate_periods(periods)

if name is None and hasattr(data, 'name'):
name = data.name

Expand All @@ -178,6 +176,7 @@ def __new__(cls, data=None, ordinal=None, freq=None, start=None, end=None,
freq = Period._maybe_convert_freq(freq)

if data is None:
# TODO: Remove this block and associated kwargs; GH#20535
if ordinal is not None:
data = np.asarray(ordinal, dtype=np.int64)
else:
Expand Down
14 changes: 14 additions & 0 deletions pandas/tests/arrays/test_datetimelike.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,20 @@ def test_int_properties(self, timedelta_index, propname):

class TestPeriodArray(object):

def test_from_object_dtype(self, period_index):
pi = period_index
arr = PeriodArrayMixin(pd.Index(pi, dtype=object))
assert list(arr) == list(pi)

arr = PeriodArrayMixin(np.array(pi, dtype=object))
assert list(arr) == list(pi)

arr = PeriodArrayMixin(list(pi))
assert list(arr) == list(pi)

arr = PeriodArrayMixin(tuple(pi))
assert list(arr) == list(pi)

def test_from_pi(self, period_index):
pi = period_index
arr = PeriodArrayMixin(pi)
Expand Down
0