Skip to content
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.

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

REF: Stop mixing DTA/TDA into DTI/TDI #24476

Merged
merged 25 commits into from
Dec 29, 2018

Conversation

jbrockmendel
Copy link
Member

@jbrockmendel jbrockmendel commented Dec 29, 2018

part of the #24024 process

along the way fixes+tests DTA.__eq__(ndarray[object]) bug

@codecov
Copy link

codecov bot commented Dec 29, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #24476      +/-   ##
==========================================
- Coverage   92.32%   92.32%   -0.01%     
==========================================
  Files         166      166              
  Lines       52298    52359      +61     
==========================================
+ Hits        48285    48339      +54     
- Misses       4013     4020       +7
Flag Coverage Δ
#multiple 90.74% <100%> (-0.01%) ⬇️
#single 43.08% <69.79%> (+0.02%) ⬆️
Impacted Files Coverage Δ
pandas/core/indexes/datetimelike.py 96.14% <100%> (-1.39%) ⬇️
pandas/core/indexes/datetimes.py 96.38% <100%> (+0.24%) ⬆️
pandas/core/arrays/datetimes.py 98.23% <100%> (-0.03%) ⬇️
pandas/core/indexes/timedeltas.py 90.65% <100%> (+0.4%) ⬆️
pandas/core/indexes/period.py 92.46% <0%> (-0.23%) ⬇️
pandas/core/arrays/datetimelike.py 95.45% <0%> (-0.2%) ⬇️
pandas/core/indexes/interval.py 95.26% <0%> (ø) ⬆️
pandas/core/arrays/interval.py 93.04% <0%> (ø) ⬆️

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 4f1c1dc...11dcef0. Read the comment docs.

@codecov
Copy link

codecov bot commented Dec 29, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #24476      +/-   ##
==========================================
- Coverage   92.32%   92.31%   -0.01%     
==========================================
  Files         166      166              
  Lines       52328    52379      +51     
==========================================
+ Hits        48310    48354      +44     
- Misses       4018     4025       +7
Flag Coverage Δ
#multiple 90.73% <100%> (-0.01%) ⬇️
#single 43.06% <74.41%> (+0.02%) ⬆️
Impacted Files Coverage Δ
pandas/core/indexes/datetimelike.py 96.23% <100%> (-1.37%) ⬇️
pandas/core/indexes/datetimes.py 96.33% <100%> (+0.19%) ⬆️
pandas/core/arrays/datetimes.py 98.41% <100%> (-0.02%) ⬇️
pandas/core/indexes/timedeltas.py 90.65% <100%> (+0.4%) ⬆️
pandas/core/indexes/period.py 92.46% <0%> (-0.23%) ⬇️
pandas/core/arrays/datetimelike.py 95.47% <0%> (-0.19%) ⬇️

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 3f83627...f6a8951. Read the comment docs.


return cls(dtarr, name=name)
dtarr = DatetimeArray._generate_range(
start, end, periods,
Copy link
Contributor

Choose a reason for hiding this comment

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

should the warning message actually be in DTA._generate_range?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, the non-deprecated way it gets called is via date_range

@@ -286,16 +281,16 @@ def __new__(cls, data=None,
verify_integrity = True

if data is None:
dtarr = DatetimeArray._generate_range(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this change be (partially) reverted? We want the warning after the array creation, in case the array creation raises an exception.

I'm not sure whether you want cls(...) or cls._simple_new right now. Probably simple_new.

Copy link
Member Author

Choose a reason for hiding this comment

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

We want the warning after the array creation, in case the array creation raises an exception.

Makes sense, will change.

I'm not sure whether you want cls(...) or cls._simple_new right now. Probably simple_new

_simple_new. It's performant and correct regardless of what becomes of the discussed changes in DTA/TDA constructors/validation.

pandas/core/indexes/datetimelike.py Show resolved Hide resolved
_has_same_tz = ea_passthrough("_has_same_tz")
__array__ = ea_passthrough("__array__")

def round(self, freq, ambiguous='raise', nonexistent='raise'):
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like you can (maybe?) remove road, floor, and ciel. I haven't run the tests, but a manual example works.

They should be dispatched via DatetimeArray._datetimelike_methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

excellent, I'll give it a shot

Copy link
Contributor

Choose a reason for hiding this comment

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

did this work out?

Copy link
Member Author

Choose a reason for hiding this comment

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

tentatively yes. we'll see if the CI agrees in a bit

Copy link
Member Author

Choose a reason for hiding this comment

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

yep


self._freq = to_offset(value)

def __getitem__(self, key):
Copy link
Contributor

Choose a reason for hiding this comment

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

In #24024 we move __getitem__ to the base DatetimeIndexOpsMixin and DatetimeArrayMixin. That seems relatively easy to do if we're already changing __getitem__ here and in TimedeltaIndex, but it may not be possible yet.

@jreback
Copy link
Contributor

jreback commented Dec 29, 2018

can you merge master

@jreback jreback added Datetime Datetime data dtype ExtensionArray Extending pandas with custom dtypes or arrays. labels Dec 29, 2018
@jreback jreback added this to the 0.24.0 milestone Dec 29, 2018
@jreback
Copy link
Contributor

jreback commented Dec 29, 2018

lgtm. @TomAugspurger

@TomAugspurger TomAugspurger merged commit 41b2b18 into pandas-dev:master Dec 29, 2018
@TomAugspurger
Copy link
Contributor

Thanks. I'll merge this into #24024.

@jbrockmendel jbrockmendel deleted the index_data2 branch December 29, 2018 20:29

_timezone = cache_readonly(DatetimeArray._timezone.fget)
is_normalized = cache_readonly(DatetimeArray.is_normalized.fget)
_resolution = cache_readonly(DatetimeArray._resolution.fget)

strftime = ea_passthrough("strftime")
Copy link
Member

Choose a reason for hiding this comment

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

This removed the docstring. Is there a TODO / issue to add it back?

return type(self)(result, name=self.name)

@property
def freq(self): # TODO: get via eadata
Copy link
Member

Choose a reason for hiding this comment

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

The same here, this had a docstring before

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
* implement _index_data parts of pandas-dev#24024

* implement _eadata, dispatch arithmetic methods to it

* dont mix DatetimeLikeArrayMixin into DatetimeIndexOpsMixin

* dont inherit TimedeltaIndex from TimedeltaArray

* dont inherit from DatetimeArray

* use ea_passthrough

* remove previously-overriden overridings

* stop double-mixing

* stop over-writing

* handle+test object arrays

* Remove unused import

* flake8 fixup

* edits per comments
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
* implement _index_data parts of pandas-dev#24024

* implement _eadata, dispatch arithmetic methods to it

* dont mix DatetimeLikeArrayMixin into DatetimeIndexOpsMixin

* dont inherit TimedeltaIndex from TimedeltaArray

* dont inherit from DatetimeArray

* use ea_passthrough

* remove previously-overriden overridings

* stop double-mixing

* stop over-writing

* handle+test object arrays

* Remove unused import

* flake8 fixup

* edits per comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Datetime Datetime data dtype ExtensionArray Extending pandas with custom dtypes or arrays.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants