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

Make Period - Period return DateOffset instead of int #21314

Merged
merged 12 commits into from Jun 29, 2018

Conversation

Projects
None yet
3 participants
@jbrockmendel
Copy link
Member

commented Jun 4, 2018

Discussed briefly in #20049.

  • closes #xxxx
  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry
@@ -557,7 +557,7 @@ def is_full(self):
return True
if not self.is_monotonic:
raise ValueError('Index is not monotonic')
values = self.values
values = self.asi8

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Jun 4, 2018

Author Member

Is the docstring for is_full correct? It looks to me like 1) it is flipped True/False and 2) the word "missing" is ambiguous since it can refer to pd.NaT/np.nan

This comment has been minimized.

Copy link
@jreback

jreback Jun 5, 2018

Contributor

feel free to update, I don't remember why this was added / what is purpose is

This comment has been minimized.

Copy link
@jreback

jreback Jun 19, 2018

Contributor

yeah I don't remember even why we have this (can follow up later)

if self.freq != other.freq:
msg = _DIFFERENT_FREQ_INDEX.format(self.freqstr, other.freqstr)
raise IncompatibleFrequency(msg)

asi8 = self.asi8
new_data = asi8 - other.ordinal
new_data = np.array([self.freq * x for x in new_data])

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Jun 4, 2018

Author Member

It might be nice to implement DateOffset * arraylike so we don't have to right this broadcasting repeatedly.

# GH#??? PeriodIndex - Period returns an object-index
# of DateOffset objects, for which we cannot use __neg__
# directly, so we have to apply it pointwise
return other.__sub__(self).map(lambda x: -x)

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Jun 4, 2018

Author Member

Would it make sense for the object-dtype Index to attempt __neg__ like this?

@codecov

This comment has been minimized.

Copy link

commented Jun 4, 2018

Codecov Report

Merging #21314 into master will decrease coverage by <.01%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #21314      +/-   ##
==========================================
- Coverage    91.9%   91.89%   -0.01%     
==========================================
  Files         154      154              
  Lines       49558    49562       +4     
==========================================
+ Hits        45545    45547       +2     
- Misses       4013     4015       +2
Flag Coverage Δ
#multiple 90.27% <71.42%> (-0.01%) ⬇️
#single 42.02% <0%> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/core/indexes/period.py 92.67% <100%> (ø) ⬆️
pandas/core/indexes/datetimelike.py 96.55% <50%> (-0.34%) ⬇️

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 45e55af...efd7c75. Read the comment docs.

@@ -557,7 +557,7 @@ def is_full(self):
return True
if not self.is_monotonic:
raise ValueError('Index is not monotonic')
values = self.values
values = self.asi8

This comment has been minimized.

Copy link
@jreback

jreback Jun 5, 2018

Contributor

feel free to update, I don't remember why this was added / what is purpose is

@@ -899,7 +899,9 @@ def __add__(self, other):
raise TypeError("cannot add {dtype}-dtype to {cls}"
.format(dtype=other.dtype,
cls=type(self).__name__))

elif is_categorical_dtype(other):

This comment has been minimized.

Copy link
@jreback

jreback Jun 5, 2018

Contributor

I think you can make this is_extension_dtype (here and below)

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Jun 6, 2018

Author Member

That should work for now, but it isn't obvious to me how it will shake out as more EA subclasses get implemented. Let me give this some thought.

return -other.__sub__(self)
# GH#21314 PeriodIndex - Period returns an object-index
# of DateOffset objects, for which we cannot use __neg__
# directly, so we have to apply it pointwise

This comment has been minimized.

Copy link
@jreback

jreback Jun 5, 2018

Contributor

is there a test that hits this explicity?

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Jun 6, 2018

Author Member

Yes. Without this we get test failures in tests.indexes.period.test_arithmetic.TestPeriodIndexSeriesMethods for
test_pi_ops, test_pi_sub_period, test_pi_sub_period_nat

@@ -29,6 +29,8 @@ 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`)
- Subtraction of :class:`Period` from :class:`Period` will return a :class:`DateOffset` object instead of an integer (:issue:`21314`)

This comment has been minimized.

Copy link
@jreback

jreback Jun 5, 2018

Contributor

can you make a new sub-section and show a previous / new behavior example.

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Jun 6, 2018

Author Member

The "can" part of that is a very good question. I'll try.

This comment has been minimized.

Copy link
@jreback

jreback Jun 19, 2018

Contributor

your subsection looks good, you don't need this note here any longer

@jreback
Copy link
Contributor

left a comment

lgtm. some minor comments. pls rebase. ping on green.

@@ -26,10 +26,52 @@ Backwards incompatible API changes

.. _whatsnew_0240.api.datetimelike:

Period Subtraction

This comment has been minimized.

Copy link
@jreback

jreback Jun 19, 2018

Contributor

can you add a reference tag here

In [4]: june - april
Out [4]: 2

Similarly, subtraction of a ``Period`` from a ``PeriodIndex`` will not return

This comment has been minimized.

Copy link
@jreback

jreback Jun 19, 2018

Contributor

will now return

@@ -29,6 +29,8 @@ 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`)
- Subtraction of :class:`Period` from :class:`Period` will return a :class:`DateOffset` object instead of an integer (:issue:`21314`)

This comment has been minimized.

Copy link
@jreback

jreback Jun 19, 2018

Contributor

your subsection looks good, you don't need this note here any longer

@@ -557,7 +557,7 @@ def is_full(self):
return True
if not self.is_monotonic:
raise ValueError('Index is not monotonic')
values = self.values
values = self.asi8

This comment has been minimized.

Copy link
@jreback

jreback Jun 19, 2018

Contributor

yeah I don't remember even why we have this (can follow up later)

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

@jbrockmendel

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2018

gentle ping

@jreback

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2018

i rebased. ping on green. (note periodically you can rebase long running PR's)

@jbrockmendel

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2018

Ping

@jreback jreback merged commit 5afb953 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!

alimcmaster1 added a commit to alimcmaster1/pandas that referenced this pull request Aug 12, 2018

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

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.