PERF: PeriodIndex.dropna, difference, take #23095

jbrockmendel · 2018-10-11T17:38:40Z

Besides the performance boost, this goes a long way towards making PeriodIndex._simple_new actually simple; avoids passing object-dtype to _simple_new

In [2]: pi_with_nas = pd.PeriodIndex(['1985Q1', 'NaT', '1985Q2'] * 1000, freq='Q')
In [3]: %timeit pi_with_nas.dropna()
The slowest run took 13.93 times longer than the fastest. This could mean that an intermediate result is being cached.
10000 loops, best of 3: 23.3 µs per loop  <-- PR
100 loops, best of 3: 4.96 ms per loop    <-- master

In [4]: pi_diff = pd.PeriodIndex(['1985Q1', 'NaT', '1985Q3'] * 1000, freq='Q')
In [5]: %timeit pi_with_nas.difference(pi_diff)
1000 loops, best of 3: 553 µs per loop  <-- PR
100 loops, best of 3: 5.29 ms per loop  <-- master

In [6]: %timeit pi_with_nas.symmetric_difference(pi_diff)
1000 loops, best of 3: 787 µs per loop  <-- PR
100 loops, best of 3: 10.3 ms per loop  <-- master

pep8speaks · 2018-10-11T17:38:47Z

Hello @jbrockmendel! Thanks for submitting the PR.

There are no PEP8 issues in the file asv_bench/benchmarks/period.py !
There are no PEP8 issues in the file pandas/core/indexes/base.py !
There are no PEP8 issues in the file pandas/core/indexes/multi.py !
There are no PEP8 issues in the file pandas/core/indexes/period.py !
There are no PEP8 issues in the file pandas/util/testing.py !
Complete extra results for this file :

file_to_check.py:2454:-258: W605 invalid escape sequence '('
file_to_check.py:2454:-255: W605 invalid escape sequence ')'

jbrockmendel · 2018-10-11T17:40:04Z

asv_bench/benchmarks/period.py

@@ -96,6 +96,8 @@ def time_value_counts(self, typ):
 class Indexing(object):

    goal_time = 0.2
+    pi_with_nas = PeriodIndex(['1985Q1', 'NaT', '1985Q2'] * 1000, freq='Q')
+    pi_diff = PeriodIndex(['1985Q1', 'NaT', '1985Q3'] * 1000, freq='Q')


setup gets called with every round of time_foo. Doing this here is analogous to calling setup_class once.

I think this is what setup_cache is for: https://asv.readthedocs.io/en/stable/writing_benchmarks.html#setup-and-teardown-functions

setup_cache pickles and then loads in each step of the loop

jbrockmendel · 2018-10-11T17:42:05Z

pandas/core/indexes/base.py

+        if is_period_dtype(self):
+            # Avoid casting to object-dtype
+            return self._ndarray_values
+        return self.values


@TomAugspurger @jorisvandenbossche is there a better name for this that might fit with EA naming conventions?

@jschendel should IntervalIndex be special-cased here?

Once we have all EAs, this will always dispatch to the EA, and then we don't need a _take_values anymore?
(if that is the case, I wouldn't care too much about the name now)

@jschendel should IntervalIndex be special-cased here?

The implementation should be good as-is for IntervalIndex; IntervalIndex.values is an IntervalArray, which has a special-cased take method. IntervalIndex._shallow_copy/_simple_new should accept an IntervalArray (which we'd get from take), so should be good on that front.

this is basically .asi8 on Indexes now, yes? Is there a reason we are not aligning on that? or changing .asi8 to this?

jorisvandenbossche · 2018-10-11T17:54:44Z

pandas/core/indexes/base.py

+        if is_period_dtype(self):
+            # Avoid casting to object-dtype
+            return self._ndarray_values
+        return self.values


Once we have all EAs, this will always dispatch to the EA, and then we don't need a _take_values anymore?
(if that is the case, I wouldn't care too much about the name now)

jorisvandenbossche · 2018-10-11T17:57:36Z

pandas/core/indexes/base.py

@@ -519,7 +519,7 @@ def _simple_new(cls, values, name=None, dtype=None, **kwargs):
    @Appender(_index_shared_docs['_shallow_copy'])
    def _shallow_copy(self, values=None, **kwargs):
        if values is None:
-            values = self.values
+            values = self._take_values


Can you explain this change? What has take to do with shallow_copy?

This is about passing i8 instead of object to _simple_new

jorisvandenbossche · 2018-10-11T17:59:46Z

pandas/core/indexes/base.py

@@ -2158,7 +2169,7 @@ def take(self, indices, axis=0, allow_fill=True,
            if allow_fill and fill_value is not None:
                msg = 'Unable to fill values because {0} cannot contain NA'
                raise ValueError(msg.format(self.__class__.__name__))
-            taken = self.values.take(indices)
+            taken = self._take_values.take(indices)


PeriodIndex can hold NA values, so how is influencing PeriodIndex?

_take_values will be iNaT in locations where values are NaT, after passing to shallow_copy/simple_new, we get NaT back

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

pandas/core/indexes/base.py

@@ -2929,7 +2940,8 @@ def difference(self, other):
        self._assert_can_do_setop(other)

        if self.equals(other):
-            return self._shallow_copy([])
+            # pass an empty array with the appropriate dtype
+            return self._shallow_copy(self[:0])


did you add a test for this?

Or this was needed due to the change in shallow_copy?

This is just because _shallow_copy calls _simple_new, and being "simple" should not be getting a list.

No, I did not add a test, but can confirm this line is hit in the tests.

jorisvandenbossche · 2018-10-11T18:16:05Z

And the same question as on the other PR: to what extent is this conflicting with the PeriodArray PR of @TomAugspurger ? Shouldn't we rather focus on finishing that one first, and then doing some perf improvements / clean-ups afterwards?

jbrockmendel · 2018-10-11T18:38:58Z

Shouldn't we rather focus on finishing that one first, and then doing some perf improvements

@jorisvandenbossche @TomAugspurger responding to many of joris's comments/questions (here and in #23093) in one place.

The PeriodIndex/PeriodArray constructors are a pain point in large part because its _simple_new is not very simple, which is why there is the lower-level _from_ordinals. Why the complication in _simple_new? Because the values being passed to it are sometimes i8 (what we actually want), sometimes ndarray[object], and sometimes PeriodIndex. The latter two cases, as it turns out, are coming from calls to _shallow_copy, exactly the ones this PR affects.

i.e. following this PR, we'll be able to clean up the PeriodIndex constructors (on top of whats in #23093)

jorisvandenbossche · 2018-10-11T18:58:56Z

@jbrockmendel to be clear, I fully agree with your analysis about _simple_new.
But the thing is that the PeriodArray PR already solves this partly (at least for PeriodIndex, complexity is mainly moved to PeriodArray), because then, it will be much more PeriodArray that will be passed around in the Index implementation, instead of sometimes object array / sometimes ordinals. And of course, we can then still try to simplify the different constructors in PeriodArray.
As another example, The PeriodArray currently does not yet solve the take issue you try to solve here, but once that PR would land, the solution for take would be completely different (implementing it on the PeriodArray, dispatching to that, and properly wrapping it again), and the other changes you have here might not be needed.

jbrockmendel · 2018-10-11T19:01:11Z

And of course, we can then still try to simplify the different constructors in PeriodArray.

That's fair. I am definitely viewing the situation as "#22862 is a big PR and simplifying things before that will make that PR smaller, review easier, etc"

codecov · 2018-10-11T19:45:53Z

Codecov Report

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

@@            Coverage Diff             @@
##           master   #23095      +/-   ##
==========================================
- Coverage    92.2%   92.19%   -0.01%     
==========================================
  Files         169      169              
  Lines       50924    50933       +9     
==========================================
+ Hits        46952    46960       +8     
- Misses       3972     3973       +1

Flag	Coverage Δ
#multiple	`90.62% <100%> (-0.01%)`	⬇️
#single	`42.3% <52.17%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/indexes/multi.py	`95.46% <100%> (+0.01%)`	⬆️
pandas/core/indexes/period.py	`93.44% <100%> (ø)`	⬆️
pandas/util/testing.py	`86.18% <100%> (ø)`	⬆️
pandas/core/indexes/base.py	`96.56% <100%> (+0.01%)`	⬆️
pandas/core/indexes/datetimes.py	`95.61% <0%> (-0.14%)`	⬇️

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...62f0f1f. Read the comment docs.

TomAugspurger · 2018-10-12T01:48:24Z

pandas/core/indexes/multi.py

+        if is_period_dtype(values):
+            take_vals = values._ndarray_values
+        else:
+            # TODO: Why is this _values where elsewhere it is values?


I'm probably responsible for using values._values here instead of .values. AFAIK, they should be equivalent for a MultiIndex (an ndarray of tuples).

OK. I'll clear this out in the next pass, assuming this goes through.

jorisvandenbossche · 2018-10-12T07:51:51Z

Sorry, I still don't see how introducing this _take_values brings us closer to our goal. The PeriodArray PR will remove that again, no?

jreback

I don't agree this will be short lived. This is a new notion, meaning we now have actually 3 ways of representing datetimelike things. The high level EA notion (e.g. an DatetimeArray), a numpy m8 array and a numpy i8 array. You are introducing common nomenclature for the 3rd.

jreback · 2018-10-12T12:48:06Z

pandas/core/indexes/base.py

+        if is_period_dtype(self):
+            # Avoid casting to object-dtype
+            return self._ndarray_values
+        return self.values


this is basically .asi8 on Indexes now, yes? Is there a reason we are not aligning on that? or changing .asi8 to this?

TomAugspurger · 2018-10-12T12:58:10Z

I think this idea is that they'll all just be `.values` once we have PeriodArray / DatetimeArray.

…

On Fri, Oct 12, 2018 at 7:50 AM Jeff Reback ***@***.***> wrote: ***@***.**** requested changes on this pull request. I don't agree this will be short lived. This is a new notion, meaning we now have ------------------------------ In pandas/core/indexes/base.py <#23095 (comment)>: > @@ -763,6 +763,17 @@ def get_values(self): """ return self.values + @Property + def _take_values(self): + """ + Values to use in `take` operation, suitable to being passed back to + _shallow_copy/_simple_new. + """ + if is_period_dtype(self): + # Avoid casting to object-dtype + return self._ndarray_values + return self.values this is basically .asi8 on Indexes now, yes? Is there a reason we are not aligning on that? or changing .asi8 to this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23095 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQHIgYuFBTeDDuieFgaOcWO-fKYWR-cks5ukJAegaJpZM4XYCM1> .

jbrockmendel · 2018-10-12T18:59:55Z

I don't agree this will be short lived.

I think this idea is that they'll all just be .values once we have PeriodArray / DatetimeArray.

I tend to agree with Tom on this one, but the more important point in my book is that this fixes the weird arguments being passed to PeriodIndex._shallow_copy which allows us to clean up the constructor mess, simplifying the PeriodArray transition.

jreback · 2018-10-14T14:14:20Z

if they will be .values then don’t introduce something new

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

Yah, I'm beginning to agree. Plenty of other related stuff to focus on. Closing.

jbrockmendel added 3 commits October 11, 2018 09:09

Avoid using object dtype for PeriodIndex methods

65b4f0f

Add asvs

ba80015

Whatsnew

f3d3321

Update GH references

62f0f1f

jbrockmendel commented Oct 11, 2018

View reviewed changes

jorisvandenbossche reviewed Oct 11, 2018

View reviewed changes

jbrockmendel mentioned this pull request Oct 11, 2018

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

Merged

TomAugspurger reviewed Oct 12, 2018

View reviewed changes

TomAugspurger approved these changes Oct 12, 2018

View reviewed changes

jreback requested changes Oct 12, 2018

View reviewed changes

jbrockmendel mentioned this pull request Oct 12, 2018

REF: Make PeriodArray an ExtensionArray #22862

Merged

jbrockmendel closed this Oct 14, 2018

jbrockmendel deleted the take_vals branch April 5, 2020 17:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PERF: PeriodIndex.dropna, difference, take #23095

PERF: PeriodIndex.dropna, difference, take #23095

jbrockmendel commented Oct 11, 2018 •

edited

Loading

pep8speaks commented Oct 11, 2018

jbrockmendel Oct 11, 2018

mroeschke Oct 11, 2018

jbrockmendel Oct 11, 2018

jbrockmendel Oct 11, 2018

jorisvandenbossche Oct 11, 2018

jschendel Oct 11, 2018

jreback Oct 12, 2018

jorisvandenbossche Oct 11, 2018

jorisvandenbossche Oct 11, 2018

jbrockmendel Oct 11, 2018

jorisvandenbossche Oct 11, 2018

jbrockmendel Oct 11, 2018

jorisvandenbossche Oct 11, 2018

jorisvandenbossche Oct 11, 2018

jbrockmendel Oct 11, 2018

jbrockmendel Oct 11, 2018

jorisvandenbossche commented Oct 11, 2018

jbrockmendel commented Oct 11, 2018

jorisvandenbossche commented Oct 11, 2018

jbrockmendel commented Oct 11, 2018

codecov bot commented Oct 11, 2018 •

edited

Loading

TomAugspurger Oct 12, 2018

jbrockmendel Oct 12, 2018

jorisvandenbossche commented Oct 12, 2018

jreback left a comment •

edited

Loading

jreback Oct 12, 2018

TomAugspurger commented Oct 12, 2018 via email

jbrockmendel commented Oct 12, 2018

jreback commented Oct 14, 2018

jbrockmendel commented Oct 14, 2018

PERF: PeriodIndex.dropna, difference, take #23095

PERF: PeriodIndex.dropna, difference, take #23095

Conversation

jbrockmendel commented Oct 11, 2018 • edited Loading

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

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

jbrockmendel commented Oct 11, 2018

jorisvandenbossche commented Oct 11, 2018

jbrockmendel commented Oct 11, 2018

codecov bot commented Oct 11, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Oct 12, 2018

jreback left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger commented Oct 12, 2018 via email

jbrockmendel commented Oct 12, 2018

jreback commented Oct 14, 2018

jbrockmendel commented Oct 14, 2018

jbrockmendel commented Oct 11, 2018 •

edited

Loading

codecov bot commented Oct 11, 2018 •

edited

Loading

jreback left a comment •

edited

Loading