BUG: rolling.count with axis=1 #26055

yhaque1213 · 2019-04-11T20:37:07Z

closes Rolling(axis='columns').count() ignores axis= keyword #13503
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry
Added axis parameter to the constructor call in the count function, fixing the original issue in Rolling(axis='columns').count() ignores axis= keyword #13503. This fix works for the example given in the issue. This does not fix the problem addressed by jorisvandenbossche, as that seems to be a type issue.

jreback · 2019-04-11T20:52:24Z

tests pls

codecov · 2019-04-11T21:18:20Z

Codecov Report

Merging #26055 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #26055      +/-   ##
==========================================
- Coverage    91.9%   91.89%   -0.01%     
==========================================
  Files         175      175              
  Lines       52485    52485              
==========================================
- Hits        48234    48231       -3     
- Misses       4251     4254       +3

Flag	Coverage Δ
#multiple	`90.45% <ø> (ø)`	⬆️
#single	`40.75% <ø> (-0.14%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/window.py	`96.39% <ø> (ø)`	⬆️
pandas/io/gbq.py	`75% <0%> (-12.5%)`	⬇️
pandas/core/frame.py	`96.79% <0%> (-0.12%)`	⬇️
pandas/util/testing.py	`90.72% <0%> (+0.1%)`	⬆️

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 a07ed59...da47224. Read the comment docs.

codecov · 2019-04-11T21:18:27Z

Codecov Report

Merging #26055 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #26055      +/-   ##
==========================================
- Coverage   91.98%   91.97%   -0.02%     
==========================================
  Files         175      175              
  Lines       52371    52379       +8     
==========================================
  Hits        48175    48175              
- Misses       4196     4204       +8

Flag	Coverage Δ
#multiple	`90.52% <ø> (-0.01%)`	⬇️
#single	`40.69% <ø> (-0.16%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/window.py	`96.39% <ø> (ø)`	⬆️
pandas/io/gbq.py	`78.94% <0%> (-10.53%)`	⬇️
pandas/core/indexing.py	`90.53% <0%> (-0.35%)`	⬇️
pandas/core/computation/engines.py	`88.33% <0%> (-0.2%)`	⬇️
pandas/core/frame.py	`96.9% <0%> (-0.12%)`	⬇️
pandas/core/indexes/base.py	`96.94% <0%> (-0.06%)`	⬇️
pandas/core/reshape/pivot.py	`96.54% <0%> (ø)`	⬆️
pandas/io/packers.py	`88.08% <0%> (ø)`	⬆️
pandas/core/indexes/interval.py	`95.22% <0%> (ø)`	⬆️
pandas/core/sparse/series.py	`93.3% <0%> (ø)`	⬆️
... and 22 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 fecee8f...d040376. Read the comment docs.

pep8speaks · 2019-04-12T02:49:49Z

Hello @yhaque1213! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-04-26 18:51:18 UTC

pandas/tests/test_window.py

yhaque1213 · 2019-04-12T23:17:11Z

@gfyoung
I updated the tests to replace my calls to assert_equal with tm.assert_frame_equal, and I also modified it to pass pep8 checks. My initial changes to fix the issue, without any tests, was passing all of the builds.

Once I added my tests, however, I started failing pandas-dev.pandas and pandas-dev.pandas (Linux py35_compat), and I'm having some trouble figuring out what's causing this to happen.

More specifically, it seems to be an error with the test_drop_duplicates_categorical_non_bool in pandas/tests/series/test_analytics.py, but I have no idea why this specific test would be failing because of adding my test, test_count_axis.

Do you have any advice?

gfyoung · 2019-04-13T01:05:23Z

@yhaque1213 : Have you merged / rebased onto the most recent master? Try that and see if it goes away. Otherwise, we'll look into seeing if we can remove (or weaken) the xfail on those tests.

yhaque1213 · 2019-04-15T22:24:42Z

@gfyoung : that seemed to do the trick - my latest update is passing all of the checks. Thank you, and please let me know if there's anything else you'd like me to add!

gfyoung · 2019-04-16T04:17:57Z

@yhaque1213 : Don't forget the whatsnew entry!

jreback

pls add a whatsnew note as well; bug fixes groupby/rolling section

jreback · 2019-04-16T13:05:49Z

pandas/tests/test_window.py

+        df_row = DataFrame({'x': [1.0, 2.0, 2.0], 'y': [1.0, 2.0, 2.0]})
+
+        # not specifiying axis and making axis=rows should be the same result
+        tm.assert_frame_equal(df.rolling(2).count(), df_row)


use

result=
expected=
tm.assert_frame_equal

so df_row looks like your expected

jreback · 2019-04-16T13:06:00Z

pandas/tests/test_window.py

+        tm.assert_frame_equal(df.rolling(2).count(), df_row)
+        tm.assert_frame_equal(df.rolling(2, axis='rows').count(), df_row)
+
+        df_col = DataFrame({'x': [1.0, 1.0, 1.0], 'y': [2.0, 2.0, 2.0]})


jreback · 2019-04-16T13:07:03Z

pandas/tests/test_window.py

@@ -667,6 +667,18 @@ def test_rolling_axis(self, axis_frame):
        result = df.rolling(3, axis=axis_frame).sum()
        tm.assert_frame_equal(result, expected)

+    def test_count_axis(self):


rename this to test_rolling_axis_count, rename the test right above to test_rolling_axis_sum

…iables

jreback · 2019-04-19T20:22:13Z

doc/source/whatsnew/v0.25.0.rst

@@ -384,6 +384,7 @@ Groupby/Resample/Rolling
 - Ensured that result group order is correct when grouping on an ordered ``Categorical`` and specifying ``observed=True`` (:issue:`25871`, :issue:`25167`)
 - Bug in :meth:`pandas.core.window.Rolling.min` and :meth:`pandas.core.window.Rolling.max` that caused a memory leak (:issue:`25893`)
 - Bug in :func:`idxmax` and :func:`idxmin` on :meth:`DataFrame.groupby` with datetime column would return incorrect dtype (:issue:`25444`, :issue:`15306`)
+- Bug in :meth:`pandas.core.window._Rolling_and_Expanding.count` that did not include axis parameter in constructor call (:issue:`13503`)


this is not a public method, use :meth:`pandas.core.window.Rolling.count and another for Expanding

jreback · 2019-04-19T20:22:40Z

doc/source/whatsnew/v0.25.0.rst

@@ -384,6 +384,7 @@ Groupby/Resample/Rolling
 - Ensured that result group order is correct when grouping on an ordered ``Categorical`` and specifying ``observed=True`` (:issue:`25871`, :issue:`25167`)
 - Bug in :meth:`pandas.core.window.Rolling.min` and :meth:`pandas.core.window.Rolling.max` that caused a memory leak (:issue:`25893`)
 - Bug in :func:`idxmax` and :func:`idxmin` on :meth:`DataFrame.groupby` with datetime column would return incorrect dtype (:issue:`25444`, :issue:`15306`)
+- Bug in :meth:`pandas.core.window._Rolling_and_Expanding.count` that did not include axis parameter in constructor call (:issue:`13503`)


this is also confusing over what you are saying, can you reword, something like '....was previously ignoring the axis keyword'

jreback · 2019-04-19T20:24:43Z

pandas/tests/test_window.py

@@ -667,6 +667,23 @@ def test_rolling_axis(self, axis_frame):
        result = df.rolling(3, axis=axis_frame).sum()
        tm.assert_frame_equal(result, expected)

+    def test_rolling_axis_count(self):


use the axis_frame fixture which resoles to rows/columns/0/1

yhaque1213 · 2019-04-19T21:33:54Z

I made the changes to the test and to the docs, but it seems like these Azure pipelines checks are passing arbitrarily. For instance, after fixing a typo in a comment (commit #12065b6) we passed 10/10 tests, while the previous commit only passed 8 (commit #41ae7fe). This happened even with rebasing before each push, therefore, we aren't sure what to change for this last commit to get it to pass all 10.

jreback · 2019-04-20T16:00:16Z

I made the changes to the test and to the docs, but it seems like these Azure pipelines checks are passing arbitrarily. For instance, after fixing a typo in a comment (commit #12065b6) we passed 10/10 tests, while the previous commit only passed 8 (commit #41ae7fe). This happened even with rebasing before each push, therefore, we aren't sure what to change for this last commit to get it to pass all 10.

CI is not 'arbitrary', rather there was an upstream change that broke things.

jreback · 2019-04-20T17:09:34Z

pls merge master and ping on green.

jreback · 2019-04-20T17:10:02Z

doc/source/whatsnew/v0.25.0.rst

@@ -384,6 +384,8 @@ Groupby/Resample/Rolling
 - Ensured that result group order is correct when grouping on an ordered ``Categorical`` and specifying ``observed=True`` (:issue:`25871`, :issue:`25167`)
 - Bug in :meth:`pandas.core.window.Rolling.min` and :meth:`pandas.core.window.Rolling.max` that caused a memory leak (:issue:`25893`)
 - Bug in :func:`idxmax` and :func:`idxmin` on :meth:`DataFrame.groupby` with datetime column would return incorrect dtype (:issue:`25444`, :issue:`15306`)
+- Bug in :meth:`pandas.core.window.Rolling.count` was previously ignoring the axis keyword (:issue:`13503`)
+- Bug in :meth:`pandas.core.window.Expanding.count` was previously ignoring the axis keyword (:issue:`13503`)


make a single entry here (just combine the lines)

jreback · 2019-04-20T17:10:34Z

pandas/tests/test_window.py

+
+        axis = df._get_axis_number(axis_frame)
+
+        if axis == 0:


this is not correct (should your test have broken?)

if axis in [0, 'index'] is what you need

…ect the axis check

…ks, trying rebase

yhaque1213 · 2019-04-21T00:32:45Z

@jreback : Just made the changes requested, and I'm passing all the checks now!

yhaque1213 · 2019-04-22T21:57:57Z

I've made all of the changes that were requested, but I'm still having trouble getting all of the checks to pass. I've tried rebasing and merging multiple times, but nothing has been working

jreback · 2019-04-22T22:06:59Z

try

git merge upstream/master

WillAyd · 2019-04-22T22:20:30Z

There's a failure in the doc checks that should be unrelated. Not sure if its a versioning thing or a commit to master. Taking a look now

jreback · 2019-04-22T22:27:45Z

this was cleared yesterday

yhaque1213 · 2019-04-22T23:17:13Z

Latest push was with merging with upstream/master, but it looks like the checks are still failing in the same ways as before. Sorry I'm not sure if I'm doing something wrong, but I've tried everything I could think of

jreback

lgtm. tiny typo. pls rebase on master which is green now.

jreback · 2019-04-23T13:05:58Z

doc/source/whatsnew/v0.25.0.rst

@@ -387,6 +387,7 @@ Groupby/Resample/Rolling
 - Ensured that ordering of outputs in ``groupby`` aggregation functions is consistent across all versions of Python (:issue:`25692`)
 - Ensured that result group order is correct when grouping on an ordered ``Categorical`` and specifying ``observed=True`` (:issue:`25871`, :issue:`25167`)
 - Bug in :meth:`pandas.core.window.Rolling.min` and :meth:`pandas.core.window.Rolling.max` that caused a memory leak (:issue:`25893`)
+- Bug in :meth:`pandas.core.window.Rolling.count` and `pandas.core. window.Expanding.count` was previously ignoring the axis keyword (:issue:`13503`)


can you remove the space here pandas.core.window....

jreback · 2019-04-26T01:18:48Z

small typo plus can i merge master ; ping on green

yhaque1213 · 2019-04-26T19:27:49Z

@jrebeck: I removed the typo - I'm passing all the checks now!

jreback · 2019-04-28T16:45:44Z

thanks @yhaque1213

Added axis to constructor call in count

da47224

TST: GH26055 added test_count_axis to TestRolling

c64c76a

gfyoung added Bug Reshaping Concat, Merge/Join, Stack/Unstack, Explode labels Apr 12, 2019

gfyoung reviewed Apr 12, 2019

View reviewed changes

pandas/tests/test_window.py Outdated Show resolved Hide resolved

jreback changed the title ~~Fixed issue #13503~~ BUG: rolling.count with axis=1 Apr 12, 2019

TST: fixed asserts and pep8 for test_count_axis()

90c6915

TST: GH26055 rebasing with master

4906502

jreback requested changes Apr 16, 2019

View reviewed changes

yhaque1213 added 3 commits April 17, 2019 20:18

TST: GH26055 added whatsnew entry and renamed result and expected var…

c6e1232

…iables

TST: GH26055 corrected doc errors and test pep8 errors

41ae7fe

TST: GH26055 fixed comment typo

12065b6

jreback requested changes Apr 19, 2019

View reviewed changes

TST: GH26055 fixed docs description and added axis_frame to test

80c8648

jreback requested changes Apr 20, 2019

View reviewed changes

yhaque1213 and others added 3 commits April 20, 2019 18:30

Combined description in doc and modified if statement in test to corr…

0f94d87

…ect the axis check

TST: GH26055 Same changes as previous commit, merge did not pass chec…

1c0f475

…ks, trying rebase

Merge branch 'master' into count-axis-fix

b0eae9e

yhaque1213 added 8 commits April 22, 2019 13:12

DOC: GH26055 removed extra doc lines

0777c54

DOC: GH26055 readded accidentally removed doc lines

6c99ab3

DOC: GH26055 removed extra space

5b87937

DOC: GH26055 merged doc conflicts

25d6cb2

Added axis to constructor call in count

73208dc

TST: GH26055 merging test conflicts

308077f

BUG: GH26055 removed extra assignment in constructor call

1265757

DOC: GH26055 removed rebase message

a23c176

yhaque1213 added 2 commits April 22, 2019 18:30

DOC: GH26055 merging doc conflicts

42c10af

DOC: GH26055 updated docs to reserve merge conflict

fff256d

jreback approved these changes Apr 23, 2019

View reviewed changes

jreback requested changes Apr 23, 2019

View reviewed changes

yhaque1213 added 8 commits April 26, 2019 14:42

DOC: GH26055 removed typo from doc

3ba5d35

Added axis to constructor call in count

9b2b960

DOC: GH26055 removed extra doc lines

b8a6cae

DOC: GH26055 readded accidentally removed doc lines

34e3018

DOC: GH26055 removed extra space

36d1c4e

BUG: GH26055 removed extra assignment in constructor call

499d910

DOC: GH26055 removed rebase message

2786a8c

DOC: GH26055 removed typo

d040376

jreback approved these changes Apr 28, 2019

View reviewed changes

jreback merged commit 48ea04f into pandas-dev:master Apr 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: rolling.count with axis=1 #26055

BUG: rolling.count with axis=1 #26055

yhaque1213 commented Apr 11, 2019 •

edited

Loading

jreback commented Apr 11, 2019

codecov bot commented Apr 11, 2019

codecov bot commented Apr 11, 2019 •

edited

Loading

pep8speaks commented Apr 12, 2019 •

edited

Loading

yhaque1213 commented Apr 12, 2019

gfyoung commented Apr 13, 2019 •

edited

Loading

yhaque1213 commented Apr 15, 2019

gfyoung commented Apr 16, 2019 •

edited

Loading

jreback left a comment

jreback Apr 16, 2019

jreback Apr 16, 2019

jreback Apr 16, 2019

jreback Apr 19, 2019

jreback Apr 19, 2019

jreback Apr 19, 2019

yhaque1213 commented Apr 19, 2019

jreback commented Apr 20, 2019

jreback commented Apr 20, 2019

jreback Apr 20, 2019

jreback Apr 20, 2019

yhaque1213 commented Apr 21, 2019

yhaque1213 commented Apr 22, 2019

jreback commented Apr 22, 2019

WillAyd commented Apr 22, 2019

jreback commented Apr 22, 2019

yhaque1213 commented Apr 22, 2019

jreback left a comment

jreback Apr 23, 2019

jreback commented Apr 26, 2019

yhaque1213 commented Apr 26, 2019

jreback commented Apr 28, 2019

BUG: rolling.count with axis=1 #26055

BUG: rolling.count with axis=1 #26055

Conversation

yhaque1213 commented Apr 11, 2019 • edited Loading

jreback commented Apr 11, 2019

codecov bot commented Apr 11, 2019

Codecov Report

codecov bot commented Apr 11, 2019 • edited Loading

Codecov Report

pep8speaks commented Apr 12, 2019 • edited Loading

Comment last updated at 2019-04-26 18:51:18 UTC

yhaque1213 commented Apr 12, 2019

gfyoung commented Apr 13, 2019 • edited Loading

yhaque1213 commented Apr 15, 2019

gfyoung commented Apr 16, 2019 • edited Loading

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

yhaque1213 commented Apr 19, 2019

jreback commented Apr 20, 2019

jreback commented Apr 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yhaque1213 commented Apr 21, 2019

yhaque1213 commented Apr 22, 2019

jreback commented Apr 22, 2019

WillAyd commented Apr 22, 2019

jreback commented Apr 22, 2019

yhaque1213 commented Apr 22, 2019

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Apr 26, 2019

yhaque1213 commented Apr 26, 2019

jreback commented Apr 28, 2019

yhaque1213 commented Apr 11, 2019 •

edited

Loading

codecov bot commented Apr 11, 2019 •

edited

Loading

pep8speaks commented Apr 12, 2019 •

edited

Loading

gfyoung commented Apr 13, 2019 •

edited

Loading

gfyoung commented Apr 16, 2019 •

edited

Loading