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.

Sign up for GitHub

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

Jump to bottom

Fixturize tests/frame/test_apply #22735

Merged
merged 5 commits into from
Sep 23, 2018
Merged

Fixturize tests/frame/test_apply #22735

merged 5 commits into from
Sep 23, 2018

Conversation

h-vetinari
Copy link
Contributor

Split off from #22730 as per review from @WillAyd

@pep8speaks
Copy link

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

@h-vetinari h-vetinari mentioned this pull request Sep 17, 2018
Merged
3 tasks
@h-vetinari h-vetinari force-pushed the fixturize_frame_apply branch from 0107e16 to bbc92b2 Compare September 17, 2018 23:41
@h-vetinari h-vetinari mentioned this pull request Sep 17, 2018
Closed
34 tasks
@h-vetinari
Copy link
Contributor Author

@gfyoung @jreback @WillAyd
Could someone retrigger the failed travis job? Thanks.

@codecov
Copy link

codecov bot commented Sep 18, 2018 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #22735      +/-   ##
==========================================
+ Coverage   92.17%   92.18%   +<.01%     
==========================================
  Files         169      169              
  Lines       50778    50815      +37     
==========================================
+ Hits        46807    46845      +38     
+ Misses       3971     3970       -1
Flag Coverage Δ
#multiple 90.6% <ø> (ø) ⬆️
#single 42.37% <ø> (+0.04%) ⬆️
Impacted Files Coverage Δ
pandas/core/dtypes/base.py 100% <0%> (ø) ⬆️
pandas/core/generic.py 96.67% <0%> (ø) ⬆️
pandas/util/testing.py 86.03% <0%> (ø) ⬆️
pandas/core/frame.py 97.2% <0%> (ø) ⬆️
pandas/core/indexes/multi.py 95.45% <0%> (ø) ⬆️
pandas/io/formats/excel.py 97.4% <0%> (ø) ⬆️
pandas/core/arrays/categorical.py 95.75% <0%> (+0.01%) ⬆️
pandas/core/base.py 97.61% <0%> (+0.01%) ⬆️
pandas/core/indexes/datetimelike.py 98.1% <0%> (+0.01%) ⬆️
pandas/core/arrays/datetimelike.py 95.53% <0%> (+0.02%) ⬆️
... and 5 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 1c113db...77bcbce. Read the comment docs.

@h-vetinari
Copy link
Contributor Author

@WillAyd Green

@gfyoung gfyoung added Refactor Internal refactoring of code Testing pandas testing functions or related to the test suite labels Sep 21, 2018
gfyoung
gfyoung approved these changes Sep 21, 2018
View reviewed changes
Copy link
Member

@gfyoung gfyoung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

cc @jreback @WillAyd

WillAyd
WillAyd requested changes Sep 21, 2018
View reviewed changes
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few minor comments but otherwise looks good

pandas/tests/frame/test_apply.py Outdated
expected = DataFrame(
np.tile(np.arange(3, dtype='int64'), 6).reshape(6, -1) + 1,
columns=['test', 'other', 'cols'])
expected = DataFrame(int_frame_const_col.values,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using the DataFrame constructor can you just rename the columns to get to the same place?

pandas/tests/frame/test_apply.py Outdated
columns=['test', 'other'])
result = df.apply(lambda x: Series([1, 2], index=['test', 'other']),
axis=1)
expected = DataFrame(int_frame_const_col.values[:, :2],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar comment as above - would be more idiomatic than slicing .values on the frame and passing to a constructor

pandas/tests/frame/test_apply.py Outdated Show resolved Hide resolved
@h-vetinari
Copy link
Contributor Author

@WillAyd @gfyoung Green :)

@jreback
Copy link
Contributor

jreback commented Sep 21, 2018

@h-vetinari i will look at all of the PRs soon

jreback
jreback requested changes Sep 21, 2018
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will look soon

@jreback jreback added this to the 0.24.0 milestone Sep 23, 2018
jreback
jreback requested changes Sep 23, 2018
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks fine. small change, ping on green.

pandas/tests/frame/conftest.py Outdated
@@ -40,6 +40,18 @@ def int_frame():
return DataFrame({c: s for c, s in compat.iteritems(df)}, dtype=np.int64)


@pytest.fixture
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so don't add this here, rather make it a fixture in test_apply.py; this conftest is for pretty common fixtures, this is not one of them (of course if could decide later to change this)

@jreback
Copy link
Contributor

jreback commented Sep 23, 2018

lgtm. ping on green.

@h-vetinari
Copy link
Contributor Author

@jreback @WillAyd @gfyoung Green. :)

@gfyoung gfyoung merged commit 44a9b16 into pandas-dev:master Sep 23, 2018
@gfyoung
Copy link
Member

gfyoung commented Sep 23, 2018

@h-vetinari : Thanks!

@h-vetinari h-vetinari deleted the fixturize_frame_apply branch September 23, 2018 21:21
Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018
@h-vetinari
TST/CLN: Fixturize tests/frame/test_apply (pandas-dev#22735)
0ebf318
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gfyoung gfyoung gfyoung approved these changes

@jreback jreback jreback approved these changes

@WillAyd WillAyd WillAyd approved these changes

Assignees
No one assigned
Labels
Refactor Internal refactoring of code Testing pandas testing functions or related to the test suite
Projects
None yet
Milestone
0.24.0
Development

Successfully merging this pull request may close these issues.
5 participants
@h-vetinari @pep8speaks @jreback @gfyoung @WillAyd