REF: Pieces broken off of #24024 #24364

jbrockmendel · 2018-12-19T23:03:20Z

If I did this right, this shouldn't cause rebasing headaches. LMK if I messed up on that and I'll try to fix.

Nearly everything here is verbatim from #24024. I'll comment inline with pieces that are not.

jbrockmendel · 2018-12-19T23:04:16Z

pandas/core/arrays/period.py

@@ -983,7 +983,7 @@ def dt64arr_to_periodarr(data, freq, tz=None):

    """
    if data.dtype != np.dtype('M8[ns]'):
-        raise ValueError('Wrong dtype: %s' % data.dtype)
+        raise ValueError('Wrong dtype: {dtype}'.format(dtype=data.dtype))


Not from #24024, but @jreback commented there asking for this change.

jbrockmendel · 2018-12-19T23:05:05Z

pandas/core/indexes/datetimes.py

+                start, end, periods,
+                freq=freq, tz=tz, normalize=normalize,
+                closed=closed, ambiguous=ambiguous)
+            return cls(dtarr, name=name)


I think in #24024 this becomes cls._simple_new(dtarr, name=name). That isn't possible yet here.

ditto below and for TimedeltaIndex

Is it worth doing this now then, when we'll just need to change it again?

The upside is that it means the diff in 24024 is just one line. I'm OK with this either way.

jbrockmendel · 2018-12-19T23:05:56Z

pandas/tests/arrays/test_timedeltas.py

+
+        arr = TimedeltaArray(data, copy=True)
+        assert arr._data is not data
+        assert arr._data.base is not data


This last line is not present in the test in #24024.

jbrockmendel · 2018-12-19T23:07:01Z

pandas/tests/indexes/datetimes/test_tools.py

@@ -246,6 +247,17 @@ def test_to_datetime_parse_timezone_keeps_name(self):


 class TestToDatetime(object):
+    def test_to_datetime_dtarr(self):


This test is not in #24024, but this tests the changes in core.tools.datetimes

jbrockmendel · 2018-12-19T23:08:45Z

In terms of LOC this shaves about 5% off of 24024. Not huge, but along with searchsorted, repeat, and reductions getting broken off, these add up

codecov · 2018-12-19T23:49:38Z

Codecov Report

Merging #24364 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #24364      +/-   ##
==========================================
+ Coverage   92.29%   92.29%   +<.01%     
==========================================
  Files         162      162              
  Lines       51832    51834       +2     
==========================================
+ Hits        47839    47841       +2     
  Misses       3993     3993

Flag	Coverage Δ
#multiple	`90.7% <100%> (ø)`	⬆️
#single	`42.99% <59.52%> (+0.01%)`	⬆️

Impacted Files	Coverage Δ
pandas/core/arrays/datetimelike.py	`96.46% <100%> (+0.01%)`	⬆️
pandas/core/indexes/datetimelike.py	`97.3% <100%> (ø)`	⬆️
pandas/core/arrays/timedeltas.py	`87.38% <100%> (+0.22%)`	⬆️
pandas/core/arrays/datetimes.py	`98.24% <100%> (ø)`	⬆️
pandas/core/indexes/timedeltas.py	`90.39% <100%> (-0.03%)`	⬇️
pandas/core/arrays/period.py	`98.5% <100%> (ø)`	⬆️
pandas/core/reshape/merge.py	`94.3% <100%> (ø)`	⬆️
pandas/core/indexes/datetimes.py	`96.32% <100%> (-0.01%)`	⬇️
pandas/core/tools/datetimes.py	`85.32% <100%> (+0.04%)`	⬆️
pandas/core/indexes/base.py	`96.27% <100%> (ø)`	⬆️
... and 2 more

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 f6cf7d9...d800f2b. Read the comment docs.

codecov · 2018-12-19T23:49:40Z

Codecov Report

Merging #24364 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #24364      +/-   ##
==========================================
+ Coverage   92.29%   92.29%   +<.01%     
==========================================
  Files         162      162              
  Lines       51832    51834       +2     
==========================================
+ Hits        47839    47841       +2     
  Misses       3993     3993

Flag	Coverage Δ
#multiple	`90.7% <100%> (ø)`	⬆️
#single	`42.99% <59.52%> (+0.01%)`	⬆️

Impacted Files	Coverage Δ
pandas/core/arrays/datetimelike.py	`96.46% <100%> (+0.01%)`	⬆️
pandas/core/indexes/datetimelike.py	`97.3% <100%> (ø)`	⬆️
pandas/core/arrays/timedeltas.py	`87.38% <100%> (+0.22%)`	⬆️
pandas/core/arrays/datetimes.py	`98.24% <100%> (ø)`	⬆️
pandas/core/indexes/timedeltas.py	`90.39% <100%> (-0.03%)`	⬇️
pandas/core/arrays/period.py	`98.5% <100%> (ø)`	⬆️
pandas/core/reshape/merge.py	`94.3% <100%> (ø)`	⬆️
pandas/core/indexes/datetimes.py	`96.32% <100%> (-0.01%)`	⬇️
pandas/core/tools/datetimes.py	`85.32% <100%> (+0.04%)`	⬆️
pandas/core/indexes/base.py	`96.27% <100%> (ø)`	⬆️
... and 2 more

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 f6cf7d9...cc3a301. Read the comment docs.

TomAugspurger · 2018-12-20T14:11:02Z

#24024 doesn't apply cleanly on top of this.

Could we instead give a +1 to the changes here, and then you make a PR against my branch for #24024 with the new changes?

jbrockmendel · 2018-12-20T17:29:48Z

Could we instead give a +1 to the changes here, and then you make a PR against my branch for #24024 with the new changes?

Will do.

jreback

i would split out the hasnans change and do that separately as it needs to happen globally

(bamboo-dev) jreback@dev:~/pandas-dev$ grep -r hasnans pandas --include '*.py'|cut -d ' ' -f 1|uniq
pandas/core/arrays/timedeltas.py:
pandas/core/arrays/period.py:
pandas/core/arrays/datetimelike.py:
pandas/core/arrays/datetimes.py:
pandas/core/indexes/timedeltas.py:
pandas/core/indexes/interval.py:
pandas/core/indexes/category.py:
pandas/core/indexes/numeric.py:
pandas/core/indexes/datetimelike.py:
pandas/core/indexes/base.py:
pandas/core/dtypes/dtypes.py:
pandas/core/series.py:
pandas/core/resample.py:
pandas/core/base.py:
pandas/tests/test_lib.py:
pandas/tests/indexes/datetimes/test_ops.py:
pandas/tests/indexes/interval/test_interval.py:
pandas/tests/indexes/period/test_ops.py:
pandas/tests/indexes/multi/test_missing.py:
pandas/tests/indexes/multi/test_missing.py:def
pandas/tests/indexes/multi/test_missing.py:
pandas/tests/indexes/common.py:
pandas/tests/indexes/timedeltas/test_ops.py:
pandas/tests/indexes/test_common.py:
pandas/tests/series/test_internals.py:def
pandas/tests/series/test_internals.py:

jreback · 2018-12-21T17:03:45Z

pandas/core/arrays/datetimelike.py

@@ -451,7 +464,7 @@ def _isnan(self):
        return (self.asi8 == iNaT)

    @property  # NB: override with cache_readonly in immutable subclasses
-    def hasnans(self):
+    def _hasnans(self):


IIRC this is a general Index property, are changing this globally? (ok by me)

TomAugspurger · 2018-12-21T17:18:22Z

What do you mean by globally changing the hasnans? I don't think anyone was suggesting changing that anywhere other than on the datelike Array classes, where we (I) don't want them as part of the public API.

…

On Fri, Dec 21, 2018 at 11:06 AM Jeff Reback ***@***.***> wrote: ***@***.**** requested changes on this pull request. i would split out the hasnans change and do that separately as it needs to happen globally (bamboo-dev) ***@***.***:~/pandas-dev$ grep -r hasnans pandas --include '*.py'|cut -d ' ' -f 1|uniq pandas/core/arrays/timedeltas.py: pandas/core/arrays/period.py: pandas/core/arrays/datetimelike.py: pandas/core/arrays/datetimes.py: pandas/core/indexes/timedeltas.py: pandas/core/indexes/interval.py: pandas/core/indexes/category.py: pandas/core/indexes/numeric.py: pandas/core/indexes/datetimelike.py: pandas/core/indexes/base.py: pandas/core/dtypes/dtypes.py: pandas/core/series.py: pandas/core/resample.py: pandas/core/base.py: pandas/tests/test_lib.py: pandas/tests/indexes/datetimes/test_ops.py: pandas/tests/indexes/interval/test_interval.py: pandas/tests/indexes/period/test_ops.py: pandas/tests/indexes/multi/test_missing.py: pandas/tests/indexes/multi/test_missing.py:def pandas/tests/indexes/multi/test_missing.py: pandas/tests/indexes/common.py: pandas/tests/indexes/timedeltas/test_ops.py: pandas/tests/indexes/test_common.py: pandas/tests/series/test_internals.py:def pandas/tests/series/test_internals.py: ------------------------------ In pandas/core/arrays/datetimelike.py <#24364 (comment)>: > @@ -451,7 +464,7 @@ def _isnan(self): return (self.asi8 == iNaT) @Property # NB: override with cache_readonly in immutable subclasses - def hasnans(self): + def _hasnans(self): IIRC this is a general Index property, are changing this globally? (ok by me) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#24364 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQHIs3VybVXypznl-NUsnhsqll10BnBks5u7RUFgaJpZM4ZbLQI> .

jbrockmendel · 2018-12-21T17:19:46Z

I don't think anyone was suggesting changing that anywhere other than on the datelike Array classes,

This matches what I had in mind, just doing it on DTI/TDI/PI.

TomAugspurger · 2018-12-21T17:26:37Z

You wrote DTI/TDI/PI, did you mean DTA/TDA/PA? Or do you mean aliasing _hasnans to hasnans on the index classes? I should probably just look at the diff :)

…

On Fri, Dec 21, 2018 at 11:19 AM jbrockmendel ***@***.***> wrote: I don't think anyone was suggesting changing that anywhere other than on the datelike Array classes, This matches what I had in mind, just doing it on DTI/TDI/PI. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#24364 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQHIluljjVWby0HQEs-9VSqTVrke2Kwks5u7Rg3gaJpZM4ZbLQI> .

jbrockmendel · 2018-12-21T17:28:32Z

Probably a typo. Lots of open windows ATM.

jreback · 2018-12-21T18:00:02Z

What do you mean by globally changing the hasnans? I don't think anyone was
suggesting changing that anywhere other than on the datelike Array classes,
where we (I) don't want them as part of the public API.

that's fine, but wont' accept a change that also does not fix the existing Index codebases. This is SO confusing you need as much consistency as possible.

TomAugspurger · 2018-12-21T18:30:03Z

I'm pretty confused right now, so I'll restate what Brock and I are saying.

The hasnans property shouldn't be part of the public API for Datetime-like arrays. It's duplicative with isna()
Index.hasnans is a well-established API that we don't want to change.

Maybe you could restate what you're looking for @jreback?

jbrockmendel · 2018-12-21T23:40:14Z

Did we ever reach a consensus here?

jreback · 2018-12-23T15:47:29Z

thanks. I realized you are just changing the non-Index idiom here which is in-line with other idioms.

jbrockmendel added 2 commits December 19, 2018 14:40

pieces broken off of pandas-dev#24024

fe31d50

add docstring

2c23624

jbrockmendel commented Dec 19, 2018

View reviewed changes

parametrize test over tz

d800f2b

isort fixup

cc3a301

This was referenced Dec 20, 2018

bits of 24364 TomAugspurger/pandas#6

Merged

REF: DatetimeLikeArray #24024

Merged

jreback requested changes Dec 21, 2018

View reviewed changes

jreback added the Internals Related to non-user accessible pandas implementation label Dec 21, 2018

jreback added this to the 0.24.0 milestone Dec 23, 2018

jreback approved these changes Dec 23, 2018

View reviewed changes

jreback merged commit d7bf6f2 into pandas-dev:master Dec 23, 2018

jbrockmendel deleted the less24024 branch December 23, 2018 16:37

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

REF: Pieces broken off of pandas-dev#24024 (pandas-dev#24364)

dd6c082

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

REF: Pieces broken off of pandas-dev#24024 (pandas-dev#24364)

33ae55e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

REF: Pieces broken off of #24024 #24364

REF: Pieces broken off of #24024 #24364

jbrockmendel commented Dec 19, 2018

jbrockmendel Dec 19, 2018

jbrockmendel Dec 19, 2018

jbrockmendel Dec 19, 2018

TomAugspurger Dec 20, 2018

jbrockmendel Dec 20, 2018

jbrockmendel Dec 19, 2018

jbrockmendel Dec 19, 2018

jbrockmendel commented Dec 19, 2018

codecov bot commented Dec 19, 2018

codecov bot commented Dec 19, 2018 •

edited

Loading

TomAugspurger commented Dec 20, 2018

jbrockmendel commented Dec 20, 2018

jreback left a comment

jreback Dec 21, 2018

TomAugspurger commented Dec 21, 2018 via email

jbrockmendel commented Dec 21, 2018

TomAugspurger commented Dec 21, 2018 via email

jbrockmendel commented Dec 21, 2018

jreback commented Dec 21, 2018

TomAugspurger commented Dec 21, 2018

jbrockmendel commented Dec 21, 2018

jreback commented Dec 23, 2018

		@@ -246,6 +247,17 @@ def test_to_datetime_parse_timezone_keeps_name(self):


		class TestToDatetime(object):
		def test_to_datetime_dtarr(self):

REF: Pieces broken off of #24024 #24364

REF: Pieces broken off of #24024 #24364

Conversation

jbrockmendel commented Dec 19, 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

jbrockmendel commented Dec 19, 2018

codecov bot commented Dec 19, 2018

Codecov Report

codecov bot commented Dec 19, 2018 • edited Loading

Codecov Report

TomAugspurger commented Dec 20, 2018

jbrockmendel commented Dec 20, 2018

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger commented Dec 21, 2018 via email

jbrockmendel commented Dec 21, 2018

TomAugspurger commented Dec 21, 2018 via email

jbrockmendel commented Dec 21, 2018

jreback commented Dec 21, 2018

TomAugspurger commented Dec 21, 2018

jbrockmendel commented Dec 21, 2018

jreback commented Dec 23, 2018

codecov bot commented Dec 19, 2018 •

edited

Loading