Fixing shift() for ExtensionArray #23947

varadgunjal · 2018-11-27T09:59:01Z

closes ExtensionArray.shift incorrect for periods larger than length of array #23911
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

pep8speaks · 2018-11-27T09:59:05Z

Hello @varadgunjal! Thanks for updating the PR.

There are no PEP8 issues in the file pandas/core/arrays/base.py !
There are no PEP8 issues in the file pandas/core/arrays/sparse.py !
There are no PEP8 issues in the file pandas/tests/extension/base/interface.py !

Comment last updated on November 27, 2018 at 20:04 Hours UTC

TomAugspurger · 2018-11-27T11:59:02Z

Looks like some of the other shift implementations were buggy: https://travis-ci.org/pandas-dev/pandas/jobs/460168964

https://travis-ci.org/pandas-dev/pandas/jobs/46016896.

Could you also update the docstring for ExtensionArray.shift adding a note about the expected behavior when periods is larger than self?

varadgunjal · 2018-11-27T12:12:15Z

So, this is the 2nd build I've seen fail. From what I understand from the logs, it looks like the previous implementation of shift is being used despite having pushed the update in the file.

varadgunjal · 2018-11-27T12:14:39Z

Oops sorry, didn't read your comment before I posted the previous one.

Could you also update the docstring for ExtensionArray.shift adding a note about the expected behavior when periods is larger than self?

Completely slipped my mind. Will do so.

Looks like some of the other shift implementations were buggy.

So, what is the correct course of action? Should I dig in further and try to fix those or leave them for a separate PR?

jreback

can you add a whatsnew note, put in the extension array section

TomAugspurger · 2018-11-27T12:31:15Z

ExtensionArray.shift is new in 0.24.0, so just need to make sure that there's an existing note there.

…

On Tue, Nov 27, 2018 at 6:30 AM Jeff Reback ***@***.***> wrote: ***@***.**** requested changes on this pull request. can you add a whatsnew note, put in the extension array section — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#23947 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQHIg08SGNonHnbSrhdVs1nTtlA3HiYks5uzTBfgaJpZM4Y1Hv0> .

varadgunjal · 2018-11-27T12:58:16Z

@TomAugspurger so I don't make an edit in whatsnew, correct? Also, how do I address the build failing on the newly added test?

TomAugspurger · 2018-11-27T16:34:34Z

@varadgunjal it seems like we don't have a dedicated release note for ExtensionArray.shift. Can you add one on ~938 and use 22387 for the issue?

varadgunjal · 2018-11-27T18:47:59Z

Just noticed 22387 refers to a PR opened by you for issue #22386. Should I ignore that and keep it as the issue?

TomAugspurger · 2018-11-27T18:56:11Z

Either works.

…

On Tue, Nov 27, 2018 at 12:48 PM Varad Gunjal ***@***.***> wrote: Just noticed 22387 refers to a PR opened by you for issue #22386 <#22386>. Should I ignore that and keep it as the issue? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23947 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQHIrRIzObvX9TXsSkq_iyx5wVjzmyEks5uzYjngaJpZM4Y1Hv0> .

varadgunjal · 2018-11-27T19:18:50Z

Cool. I still don't get what we're supposed to do about the failing build.though.

TomAugspurger · 2018-11-27T19:29:28Z

SparseArray defines SparseArray.shift. It seems it contains the same bug as the base ExtensionArray.shift, so you'll need to apply a similar fix there as well.

TomAugspurger · 2018-11-27T19:41:40Z

No, same PR. This is why we write base tests for these kinds of things, so that every extension array implementation behaves correctly. You don't need to write any new tests, since SparseArray automatically picks up the one you wrote for this PR.

…

On Tue, Nov 27, 2018 at 1:33 PM Varad Gunjal ***@***.***> wrote: 👍 Separate PR I assume? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23947 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQHItLnjNQd29IfrrtUCy_J0F5g5S3Pks5uzZOAgaJpZM4Y1Hv0> .

varadgunjal · 2018-11-28T06:05:02Z

The newest casualty is ArrowBoolArray. But I've run up against a problem when I tried to fix that, since the assert values.type == pa.bool_() constrains the values in it to be boolean. Thus, adding in an na_value violates this. So, what is the expected behavior here? Should I modify the _from_sequence implementation inside ArrowBoolArray to treat NaNs differently?

TomAugspurger · 2018-11-29T15:16:11Z

Feel free to add skips for the arrow tests. That implementation is pretty buggy right now.

codecov · 2018-12-01T04:25:05Z

Codecov Report

Merging #23947 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #23947      +/-   ##
==========================================
+ Coverage   92.31%   92.31%   +<.01%     
==========================================
  Files         161      161              
  Lines       51483    51471      -12     
==========================================
- Hits        47525    47517       -8     
+ Misses       3958     3954       -4

Flag	Coverage Δ
#multiple	`90.71% <100%> (ø)`	⬆️
#single	`42.43% <0%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/arrays/base.py	`98.01% <100%> (+0.66%)`	⬆️
pandas/core/arrays/sparse.py	`92.06% <100%> (+0.12%)`	⬆️
pandas/core/dtypes/cast.py	`89.38% <0%> (-0.25%)`	⬇️
pandas/io/json/normalize.py	`96.87% <0%> (ø)`	⬆️
pandas/tseries/offsets.py	`96.98% <0%> (ø)`	⬆️
pandas/core/indexes/datetimes.py	`96.47% <0%> (ø)`	⬆️
pandas/core/groupby/base.py	`91.83% <0%> (ø)`	⬆️
pandas/core/frame.py	`97.02% <0%> (ø)`	⬆️
pandas/core/nanops.py	`95.05% <0%> (ø)`	⬆️
pandas/io/json/json.py	`93.09% <0%> (ø)`	⬆️
... and 14 more

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 90961f2...67c268d. Read the comment docs.

codecov · 2018-12-01T04:25:06Z

Codecov Report

Merging #23947 into master will decrease coverage by 0.1%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #23947      +/-   ##
==========================================
- Coverage   92.31%    92.2%   -0.11%     
==========================================
  Files         161      162       +1     
  Lines       51483    51700     +217     
==========================================
+ Hits        47525    47671     +146     
- Misses       3958     4029      +71

Flag	Coverage Δ
#multiple	`90.6% <100%> (-0.1%)`	⬇️
#single	`43.02% <0%> (+0.58%)`	⬆️

Impacted Files	Coverage Δ
pandas/core/arrays/base.py	`97.41% <100%> (+0.06%)`	⬆️
pandas/core/arrays/sparse.py	`92.08% <100%> (+0.14%)`	⬆️
pandas/io/s3.py	`0% <0%> (-86.37%)`	⬇️
pandas/core/arrays/timedeltas.py	`87.1% <0%> (-9.34%)`	⬇️
pandas/io/parquet.py	`76.92% <0%> (-7.7%)`	⬇️
pandas/io/formats/printing.py	`85.41% <0%> (-7.67%)`	⬇️
pandas/io/formats/console.py	`74.24% <0%> (-1.52%)`	⬇️
pandas/io/common.py	`72.09% <0%> (-0.78%)`	⬇️
pandas/io/json/json.py	`92.61% <0%> (-0.48%)`	⬇️
... and 69 more

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 90961f2...31a9cc8. Read the comment docs.

varadgunjal · 2018-12-01T05:07:34Z

@TomAugspurger thanks! Pushed the required changes.

TomAugspurger

A few minor doc things. LGTM otherwise.

doc/source/whatsnew/v0.24.0.rst

pandas/core/arrays/base.py

jreback

ping on green.

pandas/core/arrays/base.py

pandas/core/arrays/sparse.py

jreback · 2018-12-02T21:05:52Z

pandas/tests/extension/arrow/test_bool.py

@@ -39,6 +39,14 @@ class TestInterface(BaseArrowTests, base.BaseInterfaceTests):
    def test_repr(self, data):
        raise pytest.skip("TODO")

+    def test_shift_non_empty_array(self, data):
+        raise pytest.skip("Skipping because the implementation of"


is there an issue for this?

I'll check through the issues list and update in a while. But the reason I added this is @ TomAugspurger's recommendation above.

Feel free to add skips for the arrow tests. That implementation is pretty buggy right now.

can you add an issue for this

jreback · 2018-12-02T21:08:06Z

pandas/tests/extension/base/interface.py

@@ -86,3 +87,25 @@ def test_isna_extension_array(self, data_missing):
            assert not na.all()

            assert na.dtype._is_boolean
+
+    @pytest.mark.parametrize('periods, indices', [


I would move this to methods.py I think (and obviously move the overrides in arrow as well)

Since moving this to methods takes it out of the BaseInterfaceTests class, I can discard the overrides in arrow, since, currently, there is no test class within it deriving from BaseMethodsTests (or BaseExtensionTests for that matter). Is that acceptable?

see my comment below

jreback · 2018-12-05T12:39:01Z

@varadgunjal can you merge master and update to comments

jreback

@varadgunjal you don't need to skip on the arrow tests any longer? or are these just not running the methods tests? ok with running those tests and then skipping for this one (if needed).

pandas/core/arrays/base.py

varadgunjal · 2018-12-08T19:33:41Z

or are these just not running the methods tests?

This is the current behavior. The implementation of ArrowBoolArray does not run the methods tests at all.

TomAugspurger · 2018-12-09T12:09:32Z

Thanks @varadgunjal!

* Fixing shift() for ExtensionArray pandas-dev#23911

Fixing shift() for ExtensionArray #23911

7ff05e7

TomAugspurger added this to the 0.24.0 milestone Nov 27, 2018

TomAugspurger added Reshaping Concat, Merge/Join, Stack/Unstack, Explode ExtensionArray Extending pandas with custom dtypes or arrays. labels Nov 27, 2018

jreback requested changes Nov 27, 2018

View reviewed changes

Edited the docstring of shift() to reflect changes.

532436e

Adding whatsnew entry for 0.24.0

6584dca

varadgunjal added 2 commits November 28, 2018 01:27

Fixing SparseArray.shift()

abba682

PEP8 fix

172545a

jreback removed this from the 0.24.0 milestone Nov 29, 2018

Adding a skip for shift for because the implementation is buggy

67c268d

TomAugspurger approved these changes Dec 1, 2018

View reviewed changes

doc/source/whatsnew/v0.24.0.rst Outdated Show resolved Hide resolved

pandas/core/arrays/base.py Outdated Show resolved Hide resolved

pandas/core/arrays/base.py Show resolved Hide resolved

Adding in changes requested by @TomAugspurger

b0c13e4

jreback requested changes Dec 1, 2018

View reviewed changes

pandas/core/arrays/base.py Show resolved Hide resolved

varadgunjal added 3 commits December 2, 2018 00:56

Bugfix: changing len to len(self)

acecc2a

Adding tests for empty array

e5b7ebc

Adding relevant skips for ArrowBoolArray

eb4eea9

jreback requested changes Dec 2, 2018

View reviewed changes

jreback added this to the 0.24.0 milestone Dec 2, 2018

Changes requested by @jreback

d659404

jreback requested changes Dec 6, 2018

View reviewed changes

pandas/core/arrays/base.py Show resolved Hide resolved

PEP8 fix

31a9cc8

TomAugspurger approved these changes Dec 9, 2018

View reviewed changes

TomAugspurger merged commit 823adcf into pandas-dev:master Dec 9, 2018

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

Fixing shift() for ExtensionArray (pandas-dev#23947)

7b16820

* Fixing shift() for ExtensionArray pandas-dev#23911

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

Fixing shift() for ExtensionArray (pandas-dev#23947)

8d23088

* Fixing shift() for ExtensionArray pandas-dev#23911

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing shift() for ExtensionArray #23947

Fixing shift() for ExtensionArray #23947

varadgunjal commented Nov 27, 2018 •

edited

Loading

pep8speaks commented Nov 27, 2018 •

edited

Loading

TomAugspurger commented Nov 27, 2018

varadgunjal commented Nov 27, 2018

varadgunjal commented Nov 27, 2018 •

edited

Loading

jreback left a comment

TomAugspurger commented Nov 27, 2018 via email

varadgunjal commented Nov 27, 2018 •

edited

Loading

TomAugspurger commented Nov 27, 2018

varadgunjal commented Nov 27, 2018

TomAugspurger commented Nov 27, 2018 via email

varadgunjal commented Nov 27, 2018

TomAugspurger commented Nov 27, 2018

TomAugspurger commented Nov 27, 2018 via email

varadgunjal commented Nov 28, 2018 •

edited

Loading

TomAugspurger commented Nov 29, 2018

codecov bot commented Dec 1, 2018

codecov bot commented Dec 1, 2018 •

edited

Loading

varadgunjal commented Dec 1, 2018

TomAugspurger left a comment

jreback left a comment

jreback Dec 2, 2018

varadgunjal Dec 5, 2018 •

edited

Loading

jreback Dec 6, 2018

jreback Dec 2, 2018

varadgunjal Dec 5, 2018

jreback Dec 6, 2018

jreback commented Dec 5, 2018

jreback left a comment

varadgunjal commented Dec 8, 2018

TomAugspurger commented Dec 9, 2018

Fixing shift() for ExtensionArray #23947

Fixing shift() for ExtensionArray #23947

Conversation

varadgunjal commented Nov 27, 2018 • edited Loading

pep8speaks commented Nov 27, 2018 • edited Loading

Comment last updated on November 27, 2018 at 20:04 Hours UTC

TomAugspurger commented Nov 27, 2018

varadgunjal commented Nov 27, 2018

varadgunjal commented Nov 27, 2018 • edited Loading

jreback left a comment

Choose a reason for hiding this comment

TomAugspurger commented Nov 27, 2018 via email

varadgunjal commented Nov 27, 2018 • edited Loading

TomAugspurger commented Nov 27, 2018

varadgunjal commented Nov 27, 2018

TomAugspurger commented Nov 27, 2018 via email

varadgunjal commented Nov 27, 2018

TomAugspurger commented Nov 27, 2018

TomAugspurger commented Nov 27, 2018 via email

varadgunjal commented Nov 28, 2018 • edited Loading

TomAugspurger commented Nov 29, 2018

codecov bot commented Dec 1, 2018

Codecov Report

codecov bot commented Dec 1, 2018 • edited Loading

Codecov Report

varadgunjal commented Dec 1, 2018

TomAugspurger left a comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

jreback Dec 2, 2018

Choose a reason for hiding this comment

varadgunjal Dec 5, 2018 • edited Loading

Choose a reason for hiding this comment

jreback Dec 6, 2018

Choose a reason for hiding this comment

jreback Dec 2, 2018

Choose a reason for hiding this comment

varadgunjal Dec 5, 2018

Choose a reason for hiding this comment

jreback Dec 6, 2018

Choose a reason for hiding this comment

jreback commented Dec 5, 2018

jreback left a comment

Choose a reason for hiding this comment

varadgunjal commented Dec 8, 2018

TomAugspurger commented Dec 9, 2018

varadgunjal commented Nov 27, 2018 •

edited

Loading

pep8speaks commented Nov 27, 2018 •

edited

Loading

varadgunjal commented Nov 27, 2018 •

edited

Loading

varadgunjal commented Nov 27, 2018 •

edited

Loading

varadgunjal commented Nov 28, 2018 •

edited

Loading

codecov bot commented Dec 1, 2018 •

edited

Loading

varadgunjal Dec 5, 2018 •

edited

Loading