CLN: res/exp and GH references in frame tests #22730

h-vetinari · 2018-09-17T00:29:23Z

EDIT2: reverted import-related clean-up due to #22730 (comment)

EDIT: after review by @WillAyd, the fixturization was split out to #22735, #22736 and #22738. Now contains only the following clean-ups, also for #22733:

Replace instances of pd. with direct imports (from review TST/CLN: series.duplicated; parametrisation; fix warning #21899 (comment)):

don’t import pd, directly import instead
Replace direct use of assert_..._equal with the tm.assert_..._equal
(from review TST/CLN: series.duplicated; parametrisation; fix warning #21899 (comment)):

use tm; don’t import assert_series_equal
Clean up usage of exp/res (from review TST/CLN: Fixturize frame/test_analytics #22733 (comment)):

Here you can use result and expected in spite of the existing code using exp
Unify references to github issues

Original post below. [END EDIT]

3 modules of TST/CLN: remove TestData from frame-tests; replace with fixtures #22471
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff

I split the commits per module, and (where possible) into fixturizing and cleaning up the imports. Other notes:

For test_apply, I added an another often-used fixture
In translating the quasi-fixtures from TestData to conftest in TST/CLN: break up & parametrize tests for df.set_index #22236, I sorted the dtypes for the columns of mixed_float_frame and mixed_int_frame, which turns out to have been a mistake. This is reverted here to be a true translation of the attribute of TestData. Otherwise, tests in the newly fixturized test_arithmetic.py would fail.

En passant, I also unified GH issue references.

pep8speaks · 2018-09-17T00:29:27Z

Hello @h-vetinari! Thanks for submitting the PR.

There are no PEP8 issues in the file pandas/tests/frame/conftest.py !
There are no PEP8 issues in the file pandas/tests/frame/test_api.py !
There are no PEP8 issues in the file pandas/tests/frame/test_apply.py !
There are no PEP8 issues in the file pandas/tests/frame/test_arithmetic.py !

h-vetinari · 2018-09-17T01:15:24Z

Can't figure out ATM why pytest cannot find some of the fixtures. Everything works fine locally...

h-vetinari · 2018-09-17T10:48:45Z

I reverted the changes in test_api for now, because I couldn't get SharedWithSparse to work for tests/sparse/frame/test_frame.py, even with a new conftest.py there. It's nevertheless interesting that some builds (https://travis-ci.org/pandas-dev/pandas/jobs/429371967, https://circleci.com/gh/pandas-dev/pandas/20582) worked out. Not sure if those jobs are skipping sparse tests...?

WillAyd

There's quite a few orthogonal changes mixed in here which I don't necessarily disagree with but as always it's much easier to logically separate the changes being made. Please keep this one focused on just implementing fixtures.

Cleaning up name-spacing and comments are fine but if you want to do them should be done in separate PRs dedicated to that task. If you can remove from this change will take another look

WillAyd · 2018-09-17T16:37:18Z

pandas/tests/frame/conftest.py

@@ -70,9 +82,10 @@ def mixed_float_frame():
    Columns are ['A', 'B', 'C', 'D'].
    """
    df = DataFrame(tm.getSeriesData())
-    df.A = df.A.astype('float16')
+    df.A = df.A.astype('float32')


What's the reasoning for changing these?

See OP:

In translating the quasi-fixtures from TestData to conftest in #22236, I sorted the dtypes for the columns of mixed_float_frame and mixed_int_frame, which turns out to have been a mistake. This is reverted here to be a true translation of the attribute of TestData. Otherwise, tests in the newly fixturized test_arithmetic.py would fail.

WillAyd · 2018-09-17T16:38:56Z

pandas/tests/frame/test_api.py

@@ -356,7 +356,7 @@ def test_axis_aliases(self):
        assert_series_equal(result, expected)

    def test_class_axis(self):
-        # https://github.com/pandas-dev/pandas/issues/18147
+        # GH 18147


Generally still ask that you try to minimize your changes as much as possible. Obviously changing comments isn't going to affect any of the code base but it still makes the diffs of your PRs unnecessarily larger, which makes reviews more difficult.

You should always look for logical separation points for smaller PRs, i.e. if you really want to clean up these comments then make a separate PR for that rather than bundling here. For this change please revert this file to master

IMO these are trivial changes that don't get done otherwise, and what's more I did get asked to do this in several PRs, so I just do it preemptively.

However, I separated the commits between fixturizing and import-cleaning, so you can review the separate commits (almost) as separate PRs.

WillAyd · 2018-09-17T16:43:50Z

pandas/tests/frame/test_apply.py

        result = df.apply(lambda x: x, axis=1)
-        assert_frame_equal(result, df)
+        tm.assert_frame_equal(result, df)


Same comment as before - I like the explicit namespacing and agree it would be good to make that consistent in this module, but better done in a separate, contained PR

As above as well:

IMO these are trivial changes that don't get done otherwise, [...] However, I separated the commits between fixturizing and import-cleaning, so you can review the separate commits (almost) as separate PRs.

h-vetinari

Thanks for the review. I separated the commits into clearly distinct parts for easier reviewing.

h-vetinari · 2018-09-17T17:35:08Z

pandas/tests/frame/conftest.py

@@ -70,9 +82,10 @@ def mixed_float_frame():
    Columns are ['A', 'B', 'C', 'D'].
    """
    df = DataFrame(tm.getSeriesData())
-    df.A = df.A.astype('float16')
+    df.A = df.A.astype('float32')


See OP:

In translating the quasi-fixtures from TestData to conftest in #22236, I sorted the dtypes for the columns of mixed_float_frame and mixed_int_frame, which turns out to have been a mistake. This is reverted here to be a true translation of the attribute of TestData. Otherwise, tests in the newly fixturized test_arithmetic.py would fail.

h-vetinari · 2018-09-17T17:35:15Z

pandas/tests/frame/test_api.py

@@ -356,7 +356,7 @@ def test_axis_aliases(self):
        assert_series_equal(result, expected)

    def test_class_axis(self):
-        # https://github.com/pandas-dev/pandas/issues/18147
+        # GH 18147


IMO these are trivial changes that don't get done otherwise, and what's more I did get asked to do this in several PRs, so I just do it preemptively.

However, I separated the commits between fixturizing and import-cleaning, so you can review the separate commits (almost) as separate PRs.

h-vetinari · 2018-09-17T17:35:55Z

pandas/tests/frame/test_apply.py

        result = df.apply(lambda x: x, axis=1)
-        assert_frame_equal(result, df)
+        tm.assert_frame_equal(result, df)


As above as well:

IMO these are trivial changes that don't get done otherwise, [...] However, I separated the commits between fixturizing and import-cleaning, so you can review the separate commits (almost) as separate PRs.

WillAyd · 2018-09-17T18:06:34Z

Even if things were done in separate commits it's still makes things harder than it needs to be to have to step through them, especially as changes / edits get made over time. Again if you could do away with anything not directly relevant to "fixturization" would be very helpful and can take another look after that

h-vetinari · 2018-09-17T20:47:41Z

@WillAyd It's something I'll take into account for future PRs. I make best-effort attempts considering the information I've gotten so far; your review contradicts that somewhat. Nevertheless, I've (grudgingly) split off #22735 and #22736.

jreback · 2018-09-17T23:34:00Z

as a general rule when changing tests

change only tests (no code)
if moving things - separate PR, don’t mix moving and changing

smaller is better - test changes have to be carefully scrutinized to make sure things are not breaking

h-vetinari · 2018-09-18T00:09:10Z

@jreback

Now as small and focused as possible after split into #22735 #22736 #22738. Will keep this PR around for doing the import clean-ups after merge.

codecov · 2018-09-23T19:20:28Z

Codecov Report

❗ No coverage uploaded for pull request base (master@66c2e5f). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #22730   +/-   ##
=========================================
  Coverage          ?   92.18%           
=========================================
  Files             ?      169           
  Lines             ?    50830           
  Branches          ?        0           
=========================================
  Hits              ?    46860           
  Misses            ?     3970           
  Partials          ?        0

Flag	Coverage Δ
#multiple	`90.6% <ø> (?)`
#single	`42.37% <ø> (?)`

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 66c2e5f...ca36942. Read the comment docs.

h-vetinari · 2018-09-23T20:03:11Z

This is now based on #22733, #22735, #22736 and #22738. The commits are nice and modular so reviewing should be easy (once the remaining two PRs are merged).

jreback · 2018-09-23T20:08:52Z

Before we do any more of this

Replace instances of pd. with direct imports (from review #21899 (comment)):
don’t import pd, directly import instead

Replace direct use of assert_...equal with the tm.assert..._equal
(from review #21899 (comment)):
use tm; don’t import assert_series_equal

I would like alint rule to see what is what (and basically list the offenders as exclusions for now).

We have been changing things like this for a long time and I think we somtimes change them back (e.g. to use the imports of assert_*, rather than tm., and to use the pd.* rather than the direct imports.

Please remove these for now (or split out to a separate PR).

h-vetinari · 2018-09-23T20:26:28Z

@jreback

I would like alint rule to see what is what (and basically list the offenders as exclusions for now).

There just needs to be a clear directive - also to write a lint rule. Furthermore, as far as I can tell from the test modules I've seen, almost all would fail both versions of that lint rule.

jreback · 2018-09-23T20:31:44Z

There just needs to be a clear directive - also to write a lint rule. Furthermore, as far as I can tell from the test modules I've seen, almost all would fail both versions of that lint rule.

of course, but that is exactly the point. let's see which iway is more common, then have a lint rule to enforce it, rather than ad-hoc conversions (which has been the policy up till now).

h-vetinari · 2018-09-23T20:44:09Z

of course, but that is exactly the point. let's see which iway is more common, then have a lint rule to enforce it, rather than ad-hoc conversions (which has been the policy up till now).

Fair enough, but that's above my pay grade. Reverting the import-related commits.

This reverts commit 855a186.

jreback · 2018-09-25T13:37:15Z

can you limit the scope of this PR as it seems to overlap greating with your other one on test_analytics. I prefer PR's that change 1 thing generally; these will be reviewed an merged much faster that longer complicated ones.

h-vetinari · 2018-09-25T19:29:26Z

@jreback

can you limit the scope of this PR as it seems to overlap greating with your other one on test_analytics.

See OP and #22730 (comment):

[...] fixturization was split out to #22735, #22736 and #22738. Now contains only the following clean-ups, also for #22733:

Better to review the parts for test_analytics and test_arithmetic after #22733 and #22736.

…ests_1

h-vetinari · 2018-10-07T22:53:58Z

@WillAyd @jreback @gfyoung

Ready again to review. Also, did someone maybe change (reduce?) my GH privileges. I can't merge fixed conflicts anymore, and can't comment on a PR without simultaneously closing it.

jreback · 2018-10-07T22:59:12Z

Ready again to review. Also, did someone maybe change (reduce?) my GH privileges. I can't merge fixed conflicts anymore, and can't comment on a PR without simultaneously closing it.

hah, not possible.

I can't merge fixed conflicts anymore,

what does this mean?

jreback

tiny issue, ping on green.

jreback · 2018-10-07T22:59:26Z

pandas/tests/frame/test_analytics.py

-        # GH PR #22298
-        df = pd.DataFrame(np.random.normal(size=(10, 2)))
+        # GH 22298
+        df = pd. DataFrame(np.random.normal(size=(10, 2)))


space added here

h-vetinari · 2018-10-07T23:14:33Z

I can't merge fixed conflicts anymore,
what does this mean?

I fixed the merge conflict in the github UI, but there was no button to actually merge them. I've already restarted my browser and everything, but my "comment" button has gone (leaving only "close and comment"). Really annoying.

h-vetinari · 2018-10-07T23:15:05Z

I can't merge fixed conflicts anymore,

what does this mean?

I fixed the merge conflict in the github UI, but there was no button to actually merge them. I've already restarted my browser and everything, but my "comment" button has gone (leaving only "close and comment"). Really annoying.

EDIT: and also, I can't edit posts anymore.

jreback · 2018-10-07T23:17:11Z

@h-vetinari I am surprised you use the github UI to do anything. I have no idea what controls are on that, generally much better to use the command line.

jreback · 2018-10-07T23:18:08Z

lgtm. @WillAyd merge when satisfied.

h-vetinari · 2018-10-07T23:20:56Z

@h-vetinari I am surprised you use the github UI to do anything. I have no idea what controls are on that, generally much better to use the command line.

For tiny conflicts (handful of lines) it's quicker for me. Rest I do manually or in IDE. However, you might wanna click on a SHA hash in one of these threads sometimes, especially if there's many commits - it allows you to easily browse through the commits one by one (which is how I split up #22733).

WillAyd · 2018-10-08T03:26:08Z

thanks @h-vetinari !

h-vetinari force-pushed the fixturize_frame_tests_1 branch from 5d16b52 to 6fc3d84 Compare September 17, 2018 00:38

This was referenced Sep 17, 2018

TST/CLN: Fixturize frame/test_analytics #22733

Merged

TST/CLN: remove TestData from frame-tests; replace with fixtures #22471

Closed

h-vetinari force-pushed the fixturize_frame_tests_1 branch from 7f60806 to ef77c8f Compare September 17, 2018 10:46

WillAyd requested changes Sep 17, 2018

View reviewed changes

WillAyd added Testing pandas testing functions or related to the test suite Clean labels Sep 17, 2018

h-vetinari commented Sep 17, 2018

View reviewed changes

This was referenced Sep 17, 2018

Fixturize tests/frame/test_apply #22735

Merged

Fixturize tests/frame/test_arithmetic #22736

Merged

h-vetinari mentioned this pull request Sep 17, 2018

Fixturize tests/frame/test_api and tests/sparse/frame/test_frame #22738

Merged

3 tasks

h-vetinari force-pushed the fixturize_frame_tests_1 branch from ef77c8f to afbebb2 Compare September 23, 2018 19:20

h-vetinari changed the title ~~TST/CLN: Fixturize frame tests~~ CLN: imports etc. in frame tests Sep 23, 2018

h-vetinari added 2 commits September 23, 2018 22:09

Fixturize tests/frame/test_arithmetic

f699cd2

Fixturize tests/frame/test_analytics

e903579

h-vetinari force-pushed the fixturize_frame_tests_1 branch from afbebb2 to 8576b9c Compare September 23, 2018 20:16

Clean res/exp in frame/test_apply.py

be4605d

h-vetinari added 2 commits September 23, 2018 23:43

Change GH reference formatting (review jreback)

855a186

Revert "Change GH reference formatting (review jreback)"

733b889

This reverts commit 855a186.

h-vetinari mentioned this pull request Oct 6, 2018

CLN: Use consistent imports in tests #23018

Closed

Merge remote-tracking branch 'upstream/master' into fixturize_frame_t…

d0c6d37

…ests_1

h-vetinari force-pushed the fixturize_frame_tests_1 branch from 16a5297 to d0c6d37 Compare October 7, 2018 22:51

h-vetinari closed this Oct 7, 2018

h-vetinari reopened this Oct 7, 2018

jreback requested changes Oct 7, 2018

View reviewed changes

jreback added this to the 0.24.0 milestone Oct 7, 2018

Fix typo

ca36942

h-vetinari closed this Oct 7, 2018

h-vetinari reopened this Oct 7, 2018

h-vetinari closed this Oct 7, 2018

h-vetinari reopened this Oct 7, 2018

jreback approved these changes Oct 7, 2018

View reviewed changes

h-vetinari closed this Oct 7, 2018

h-vetinari reopened this Oct 7, 2018

WillAyd merged commit b0a2667 into pandas-dev:master Oct 8, 2018

h-vetinari deleted the fixturize_frame_tests_1 branch October 8, 2018 17:09

tm9k1 pushed a commit to tm9k1/pandas that referenced this pull request Nov 19, 2018

CLN: res/exp and GH references in frame tests (pandas-dev#22730)

32563e3

CLN: res/exp and GH references in frame tests #22730

CLN: res/exp and GH references in frame tests #22730

Conversation

h-vetinari commented Sep 17, 2018 • edited Loading

pep8speaks commented Sep 17, 2018

h-vetinari commented Sep 17, 2018

h-vetinari commented Sep 17, 2018

WillAyd 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

h-vetinari 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

WillAyd commented Sep 17, 2018

h-vetinari commented Sep 17, 2018

jreback commented Sep 17, 2018

h-vetinari commented Sep 18, 2018

codecov bot commented Sep 23, 2018 • edited Loading

Codecov Report

h-vetinari commented Sep 23, 2018 • edited Loading

jreback commented Sep 23, 2018

h-vetinari commented Sep 23, 2018

jreback commented Sep 23, 2018

h-vetinari commented Sep 23, 2018

jreback commented Sep 25, 2018

h-vetinari commented Sep 25, 2018

h-vetinari commented Oct 7, 2018

jreback commented Oct 7, 2018

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-vetinari commented Oct 7, 2018

h-vetinari commented Oct 7, 2018

jreback commented Oct 7, 2018

jreback commented Oct 7, 2018

h-vetinari commented Oct 7, 2018

WillAyd commented Oct 8, 2018

h-vetinari commented Sep 17, 2018 •

edited

Loading

codecov bot commented Sep 23, 2018 •

edited

Loading

h-vetinari commented Sep 23, 2018 •

edited

Loading