API: PeriodIndex subtraction to return object Index of DateOffsets #20049

jbrockmendel · 2018-03-08T02:45:24Z

Implements _sub_period_array in DatetimeIndexOpsMixin. Behavior is analogous to subtraction of Period scalar.

closes ENH/API: Decide what to return Period - Period subtraction #13077
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

jschendel · 2018-03-08T05:32:56Z

pandas/core/indexes/datetimelike.py

+ cls=type(self).__name__))
+
+ if not len(self) == len(other):
+ raise ValueError("cannot add indices of unequal length")


I might have missed it, but is there a test that hits this? And should it say "subtract" instead of "add"?

Good catches on both counts. Will fix.

Just pushed with fix to add-->subtract. test_pi_sub_pi, test_pi_sub_pi_with_nat, test_pi_sub_isub_pi each go through this method, though I don't think any go through this particular line

jschendel · 2018-03-08T05:33:12Z

doc/source/whatsnew/v0.23.0.txt

 - ``Categorical.fillna`` now validates its ``value`` and ``method`` keyword arguments. It now raises when both or none are specified, matching the behavior of :meth:`Series.fillna` (:issue:`19682`)
 - ``pd.to_datetime('today')`` now returns a datetime, consistent with ``pd.Timestamp('today')``; previously ``pd.to_datetime('today')`` returned a ``.normalized()`` datetime (:issue:`19935`)
 - :func:`Series.str.replace` now takes an optional `regex` keyword which, when set to ``False``, uses literal string replacement rather than regex replacement (:issue:`16808`)
+- :class:`PeriodIndex` subtraction of another ``PeriodIndex`` will now return a numeric ``Index`` instead of raising a ``TypeErrorr`` (:issue:`20049`)


Extra "r" at the end of "TypeErrorr"

jreback · 2018-03-08T11:42:20Z

I think we should be returning an array (Index?) of offsets rather than the integers which are untyped.

we don't really support this so an object Index would be ok I think.

jbrockmendel · 2018-03-08T15:34:34Z

I think we should be returning an array (Index?) of offsets rather than the integers which are untyped.

I more-or-less agree, but ATM Period - Period returns an integer and PeriodIndex - Period returns an integer array. These three operations should all be consistent.

jreback · 2018-03-10T12:32:39Z

I more-or-less agree, but ATM Period - Period returns an integer and PeriodIndex - Period returns an integer array. These three operations should all be consistent.

and these should changed (both). yes its an API breaking change but much more intuitve / better. it will just have to be an object array but that's ok.

…r_array

jbrockmendel · 2018-03-18T17:31:44Z

and these should changed (both). yes its an API breaking change but much more intuitve / better. it will just have to be an object array but that's ok.

~~Implementing this, the non-obvious issue that comes up is what to do with NaTs in a PeriodIndex when subtracting Period.~~ Never mind, this is easy to handle.

…r_array

codecov · 2018-06-04T17:09:59Z

Codecov Report

Merging #20049 into master will increase coverage by <.01%.
The diff coverage is 94.44%.

@@ Coverage Diff @@ ## master #20049 +/- ## ========================================== + Coverage 91.9% 91.9% +<.01%  ========================================== Files 153 153 Lines 49549 49565 +16 ========================================== + Hits 45537 45554 +17  + Misses 4012 4011 -1

Flag	Coverage Δ
#multiple	`90.3% <94.44%> (ø)`	⬆️
#single	`41.76% <11.11%> (-0.02%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/indexes/datetimelike.py	`96.8% <94.44%> (-0.09%)`	⬇️
pandas/util/testing.py	`85.27% <0%> (+0.2%)`	⬆️

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 f1ffc5f...0886917. Read the comment docs.

jreback

looks pretty good. rebase, small comments. @jschendel can you give a once-over

jreback · 2018-06-05T10:56:33Z

doc/source/whatsnew/v0.23.0.txt

 - :func:`DataFrame.to_dict` with ``orient='index'`` no longer casts int columns to float for a DataFrame with only int and float columns (:issue:`18580`)
 - A user-defined-function that is passed to :func:`Series.rolling().aggregate() <pandas.core.window.Rolling.aggregate>`, :func:`DataFrame.rolling().aggregate() <pandas.core.window.Rolling.aggregate>`, or its expanding cousins, will now *always* be passed a ``Series``, rather than a ``np.array``; ``.apply()`` only has the ``raw`` keyword, see :ref:`here <whatsnew_0230.enhancements.window_raw>`. This is consistent with the signatures of ``.aggregate()`` across pandas (:issue:`20584`)
 - Rolling and Expanding types raise ``NotImplementedError`` upon iteration (:issue:`11704`).
+>>>>>>> cbec58eacd8e9cd94b7f42351b8de4559c250909


jreback · 2018-06-05T10:59:26Z

pandas/tests/indexes/datetimes/test_arithmetic.py

 with pytest.raises(TypeError):
 p - idx

+ @pytest.mark.parametrize('op', [operator.add, ops.radd,


can you add (maybe another test) add/sub with a pi and another dtype (I don't think we have this test?)

Just checking, you mean to add cases over in the period test_arithmetic file, not here, right?

jreback · 2018-06-05T10:59:52Z

pandas/tests/indexes/period/test_arithmetic.py

+ rng = pd.period_range('1/1/2000', freq='D', periods=5)
+ other = pd.period_range('1/6/2000', freq='D', periods=5)
+
+ # previously performed setop union, now raises TypeError (GH14164)


this is a pretty old comment, can you improve

jreback · 2018-06-05T11:00:10Z

pandas/tests/indexes/period/test_arithmetic.py

 class TestPeriodIndexArithmetic(object):
+ # ---------------------------------------------------------------
+ # __add__/__sub__ with PeriodIndex
+


can you give a 1-2 line on what are allowed ops on PI

jreback · 2018-06-05T11:00:29Z

pandas/tests/indexes/period/test_arithmetic.py

- # TODO needs to wait on #13077 for decision on result type
+ def test_pi_sub_isub_pi(self):
+ # GH#20049
+ # previously raised TypeError (GH#14164), before that


can you refresh comment

…r_array

pep8speaks · 2018-06-06T01:36:10Z

Hello @jbrockmendel! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on June 22, 2018 at 03:51 Hours UTC

…r_array

jbrockmendel · 2018-06-11T16:22:57Z

Removing test_pi_sub_pi because it is entirely redundant with test_pi_sub_isub_pi. Moving the latter to where it should be in the test file.

jorisvandenbossche

Shouldn't first the Period scalar substraction be changed as well?

jbrockmendel · 2018-06-12T16:23:10Z

@jorisvandenbossche yes, thats in #21314.

…r_array

jschendel · 2018-06-22T03:21:45Z

doc/source/whatsnew/v0.24.0.txt


 - For :class:`DatetimeIndex` and :class:`TimedeltaIndex` with non-``None`` ``freq`` attribute, addition or subtraction of integer-dtyped array or ``Index`` will return an object of the same class (:issue:`19959`)
 - :class:`DateOffset` objects are now immutable. Attempting to alter one of these will now raise ``AttributeError`` (:issue:`21341`)
+- :class:`PeriodIndex` subtraction of another ``PeriodIndex`` will now return an object-dtype :class:`Index` of :class:`DateOffset` objects instead of raising a ``TypeErrorr`` (:issue:`20049`)


Extra "r" at the end of "TypeErrorr"

jschendel · 2018-06-22T04:11:03Z

pandas/tests/indexes/period/test_arithmetic.py

+ def test_pi_sub_pi_with_nat(self):
+ rng = pd.period_range('1/1/2000', freq='D', periods=5)
+ other = rng[1:].insert(0, pd.NaT)
+ assert other[1:].equals(rng[1:])


Is this assert necessary? I don't immediately see why it's needed, but could be missing something. If it is needed, can assert_index_equal be used instead?

This is checking that I haven't already screwed up, i.e. that the test below is indeed testing what I think it is testing.

jbrockmendel · 2018-06-26T17:46:45Z

gentle ping (this should be tandem with #21314)

jreback · 2018-06-29T00:46:05Z

thanks @jbrockmendel

also pls have a look at the period docs to see if we need updating anywhere.

…andas-dev#20049)

jbrockmendel added 2 commits March 7, 2018 18:43

implement add/sub for period_dtype other

7563876

add GH reference

14f8d86

jschendel reviewed Mar 8, 2018

View reviewed changes

jreback added Frequency DateOffsets Period Period data type labels Mar 8, 2018

Merge branch 'master' of https://github.com/pandas-dev/pandas into pe…

a623c5a

…r_array

jbrockmendel mentioned this pull request Jun 4, 2018

Make Period - Period return DateOffset instead of int #21314

Merged

4 tasks

jbrockmendel added 2 commits June 4, 2018 08:02

Merge branch 'master' of https://github.com/pandas-dev/pandas into pe…

24ebba2

…r_array

change return type from Int64Index to Index[DateOffset]

f2dac59

jreback requested changes Jun 5, 2018

View reviewed changes

jreback added this to the 0.24.0 milestone Jun 5, 2018

jbrockmendel added 3 commits June 5, 2018 09:54

Merge branch 'master' of https://github.com/pandas-dev/pandas into pe…

945ed39

…r_array

remove merge leftover

0adf604

flesh out comments

eb6ec2a

jbrockmendel added 3 commits June 5, 2018 18:37

fixup too-long line

e609631

Merge branch 'master' of https://github.com/pandas-dev/pandas into pe…

e3cfc26

…r_array

Fixup typo add-->subtract

59b314f

remove redundant test

0f6d67d

jorisvandenbossche changed the title ~~implement add/sub for period_dtype other~~ API: PeriodIndex subtraction to return object Index of DateOffsets Jun 12, 2018

jorisvandenbossche reviewed Jun 12, 2018

View reviewed changes

Merge branch 'master' of https://github.com/pandas-dev/pandas into pe…

82810d3

…r_array

Merge branch 'master' of https://github.com/pandas-dev/pandas into pe…

6637a4f

…r_array

jschendel reviewed Jun 22, 2018

View reviewed changes

fixup typo

0886917

jschendel reviewed Jun 22, 2018

View reviewed changes

jreback approved these changes Jun 29, 2018

View reviewed changes

jreback merged commit a3e56f2 into pandas-dev:master Jun 29, 2018

Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018

API: PeriodIndex subtraction to return object Index of DateOffsets (p…

b3af8eb

…andas-dev#20049)

jbrockmendel deleted the per_array branch April 5, 2020 17:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

API: PeriodIndex subtraction to return object Index of DateOffsets #20049

API: PeriodIndex subtraction to return object Index of DateOffsets #20049

Uh oh!

jbrockmendel commented Mar 8, 2018

jschendel Mar 8, 2018

jbrockmendel Mar 8, 2018

jbrockmendel Jun 11, 2018

jschendel Mar 8, 2018 •

edited

Loading

jreback commented Mar 8, 2018

jbrockmendel commented Mar 8, 2018

jreback commented Mar 10, 2018

jbrockmendel commented Mar 18, 2018 •

edited

Loading

codecov bot commented Jun 4, 2018 •

edited

Loading

jreback left a comment

jreback Jun 5, 2018

jreback Jun 5, 2018

jbrockmendel Jun 5, 2018

jreback Jun 5, 2018

jreback Jun 5, 2018

jreback Jun 5, 2018

pep8speaks commented Jun 6, 2018 •

edited

Loading

jbrockmendel commented Jun 11, 2018

jorisvandenbossche left a comment

jbrockmendel commented Jun 12, 2018

jschendel Jun 22, 2018

jschendel Jun 22, 2018

jbrockmendel Jun 22, 2018

jbrockmendel commented Jun 26, 2018

jreback commented Jun 29, 2018

Labels

5 participants

Uh oh!

API: PeriodIndex subtraction to return object Index of DateOffsets #20049

API: PeriodIndex subtraction to return object Index of DateOffsets #20049

Uh oh!

Conversation

jbrockmendel commented Mar 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jschendel Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

jreback commented Mar 8, 2018

jbrockmendel commented Mar 8, 2018

jreback commented Mar 10, 2018

jbrockmendel commented Mar 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

codecov bot commented Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

jreback 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pep8speaks commented Jun 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated on June 22, 2018 at 03:51 Hours UTC

jbrockmendel commented Jun 11, 2018

jorisvandenbossche left a comment

Choose a reason for hiding this comment

jbrockmendel commented Jun 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Jun 26, 2018

jreback commented Jun 29, 2018

Labels

5 participants

jschendel Mar 8, 2018 •

edited

Loading

jbrockmendel commented Mar 18, 2018 •

edited

Loading

codecov bot commented Jun 4, 2018 •

edited

Loading

pep8speaks commented Jun 6, 2018 •

edited

Loading