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

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

Merged
merged 15 commits into from Jun 29, 2018

Conversation

Projects
None yet
5 participants
@jbrockmendel
Copy link
Member

commented Mar 8, 2018

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

  • closes #13077
  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

jbrockmendel added some commits Mar 8, 2018

cls=type(self).__name__))

if not len(self) == len(other):
raise ValueError("cannot add indices of unequal length")

This comment has been minimized.

Copy link
@jschendel

jschendel Mar 8, 2018

Member

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

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Mar 8, 2018

Author Member

Good catches on both counts. Will fix.

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Jun 11, 2018

Author Member

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

@@ -710,6 +710,7 @@ Other API Changes
- ``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`)

This comment has been minimized.

Copy link
@jschendel

jschendel Mar 8, 2018

Member

Extra "r" at the end of "TypeErrorr"

@jreback

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Mar 8, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2018

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.

@jbrockmendel

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2018

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.

@jbrockmendel jbrockmendel referenced this pull request Jun 4, 2018

Merged

Make Period - Period return DateOffset instead of int #21314

2 of 4 tasks complete
@codecov

This comment has been minimized.

Copy link

commented Jun 4, 2018

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Contributor

left a comment

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

@@ -996,6 +996,7 @@ Other API Changes
- :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

This comment has been minimized.

Copy link
@jreback

jreback Jun 5, 2018

Contributor

leftover

@@ -740,6 +740,17 @@ def test_sub_period(self, freq):
with pytest.raises(TypeError):
p - idx

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

This comment has been minimized.

Copy link
@jreback

jreback Jun 5, 2018

Contributor

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

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Jun 5, 2018

Author Member

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

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 comment has been minimized.

Copy link
@jreback

jreback Jun 5, 2018

Contributor

this is a pretty old comment, can you improve

@@ -258,6 +258,44 @@ def test_comp_nat(self, dtype):


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

This comment has been minimized.

Copy link
@jreback

jreback Jun 5, 2018

Contributor

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

# 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

This comment has been minimized.

Copy link
@jreback

jreback Jun 5, 2018

Contributor

can you refresh comment

@jreback jreback added this to the 0.24.0 milestone Jun 5, 2018

@pep8speaks

This comment has been minimized.

Copy link

commented Jun 6, 2018

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
@jbrockmendel

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2018

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 jorisvandenbossche changed the title implement add/sub for period_dtype other API: PeriodIndex subtraction to return object Index of DateOffsets Jun 12, 2018

@jorisvandenbossche
Copy link
Member

left a comment

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

@jbrockmendel

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2018

@jorisvandenbossche yes, thats in #21314.

@@ -68,6 +68,7 @@ Datetimelike API Changes

- 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`)

This comment has been minimized.

Copy link
@jschendel

jschendel Jun 22, 2018

Member

Extra "r" at the end of "TypeErrorr"

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:])

This comment has been minimized.

Copy link
@jschendel

jschendel Jun 22, 2018

Member

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 comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Jun 22, 2018

Author Member

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

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2018

gentle ping (this should be tandem with #21314)

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

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jreback

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2018

thanks @jbrockmendel

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.