Make Period - Period return DateOffset instead of int #21314

jbrockmendel · 2018-06-04T14:42:47Z

Discussed briefly in #20049.

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

jbrockmendel · 2018-06-04T14:45:36Z

pandas/core/indexes/period.py

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


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

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

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

jbrockmendel · 2018-06-04T14:46:34Z

pandas/core/indexes/period.py

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


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

jbrockmendel · 2018-06-04T14:47:17Z

pandas/_libs/tslibs/period.pyx

+                # 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)


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

codecov · 2018-06-04T16:08:03Z

Codecov Report

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

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

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

pandas/core/indexes/period.py

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


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

jreback · 2018-06-05T10:54:32Z

pandas/core/indexes/datetimelike.py

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


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

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.

jreback · 2018-06-05T10:55:28Z

pandas/_libs/tslibs/period.pyx

-                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


is there a test that hits this explicity?

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

jreback · 2018-06-05T10:55:44Z

doc/source/whatsnew/v0.24.0.txt

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


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

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

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

…rsub

jreback

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

jreback · 2018-06-19T00:44:06Z

doc/source/whatsnew/v0.24.0.txt

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

 .. _whatsnew_0240.api.datetimelike:

+Period Subtraction


can you add a reference tag here

jreback · 2018-06-19T00:44:26Z

doc/source/whatsnew/v0.24.0.txt

+    In [4]: june - april
+    Out [4]: 2
+
+Similarly, subtraction of a ``Period`` from a ``PeriodIndex`` will not return


will now return

jreback · 2018-06-19T00:45:00Z

doc/source/whatsnew/v0.24.0.txt

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


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

jreback · 2018-06-19T00:45:43Z

pandas/core/indexes/period.py

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


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

…rsub

…ection

…rsub

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

gentle ping

jreback · 2018-06-26T22:57:53Z

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

jbrockmendel · 2018-06-27T14:26:53Z

Ping

jreback · 2018-06-29T00:41:03Z

thanks!

update tests to reflect changed Period subtraction return type

0037273

jbrockmendel commented Jun 4, 2018

View reviewed changes

update GH reference in comments and whatsnew

b981d32

jbrockmendel mentioned this pull request Jun 4, 2018

[DISC] Index[object] ops #21315

Closed

update frame Period __sub__ test

5be05ee

jschendel added Frequency DateOffsets Period Period data type labels Jun 4, 2018

jreback requested changes Jun 5, 2018

View reviewed changes

jbrockmendel added 3 commits June 5, 2018 18:20

update docstring

6c1f5f9

period subtraction docs section

3f1b65c

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

ac7b6b5

…rsub

jbrockmendel mentioned this pull request Jun 12, 2018

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

Merged

4 tasks

jreback approved these changes Jun 19, 2018

View reviewed changes

jreback added this to the 0.24.0 milestone Jun 19, 2018

jbrockmendel added 4 commits June 18, 2018 17:50

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

108f179

…rsub

remove redundant whatsnew entry, typo fixup, add period_subtraction s…

d205afc

…ection

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

5d0b301

…rsub

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

1572d6a

…rsub

jreback added 2 commits June 26, 2018 18:55

Merge branch 'master' into PR_TOOL_MERGE_PR_21314

3e0ac68

minor doc

efd7c75

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

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

Make Period - Period return DateOffset instead of int (pandas-dev#21314)

e61b7b6

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Period - Period return DateOffset instead of int #21314

Make Period - Period return DateOffset instead of int #21314

jbrockmendel commented Jun 4, 2018 •

edited

Loading

jbrockmendel Jun 4, 2018

jreback Jun 5, 2018

jreback Jun 19, 2018

jbrockmendel Jun 4, 2018

jbrockmendel Jun 4, 2018

codecov bot commented Jun 4, 2018 •

edited

Loading

jreback Jun 5, 2018

jreback Jun 5, 2018

jbrockmendel Jun 6, 2018

jreback Jun 5, 2018

jbrockmendel Jun 6, 2018

jreback Jun 5, 2018

jbrockmendel Jun 6, 2018

jreback Jun 19, 2018

jreback left a comment

jreback Jun 19, 2018

jreback Jun 19, 2018

jreback Jun 19, 2018

jreback Jun 19, 2018

jbrockmendel commented Jun 26, 2018

jreback commented Jun 26, 2018

jbrockmendel commented Jun 27, 2018

jreback commented Jun 29, 2018

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

		.. _whatsnew_0240.api.datetimelike:

		Period Subtraction

Make Period - Period return DateOffset instead of int #21314

Make Period - Period return DateOffset instead of int #21314

Conversation

jbrockmendel commented Jun 4, 2018 • edited Loading

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

codecov bot commented Jun 4, 2018 • edited Loading

Codecov Report

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

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

jbrockmendel commented Jun 26, 2018

jreback commented Jun 26, 2018

jbrockmendel commented Jun 27, 2018

jreback commented Jun 29, 2018

jbrockmendel commented Jun 4, 2018 •

edited

Loading

codecov bot commented Jun 4, 2018 •

edited

Loading