Skip to content

add a test for loc method; check if a warning raise when replacing a … #36486

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
Merged
merged 11 commits into from
Sep 22, 2020
Merged

add a test for loc method; check if a warning raise when replacing a … #36486

merged 11 commits into from
Sep 22, 2020

Conversation

samilAyoub
Copy link
Contributor

@samilAyoub samilAyoub commented Sep 19, 2020 •
edited
Loading

…subframe

@pep8speaks
Copy link

pep8speaks commented Sep 19, 2020 •
edited
Loading

Hello @samilAyoub! 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 2020-09-22 11:20:12 UTC

arw2019
arw2019 suggested changes Sep 19, 2020
View reviewed changes
Copy link
Member

@arw2019 arw2019 left a comment

Choose a reason for hiding this comment

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

thanks @samilAyoub for the PR!

some comments, o/w lgtm

pandas/tests/indexing/test_loc.py Outdated
df2 = df1.copy()
df2[:] = np.nan
# it fail if a warning is not raised
with pytest.warns(Warning):
Copy link
Member

Choose a reason for hiding this comment

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

we use tm.assert_produces_warning instead of pytest.warns

pandas/pandas/_testing.py

Line 2539 in d38dc06

def assert_produces_warning(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

pandas/tests/indexing/test_loc.py Outdated
@@ -1130,3 +1130,16 @@ def test_loc_with_period_index_indexer():
tm.assert_frame_equal(df, df.loc[list(idx)])
tm.assert_frame_equal(df.iloc[0:5], df.loc[idx[0:5]])
tm.assert_frame_equal(df, df.loc[list(idx)])

def test_loc_replace_subset_with_another():
Copy link
Member

Choose a reason for hiding this comment

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

I'd call it test_loc_replace_subset_with_subset

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@samilAyoub
Copy link
Contributor Author

samilAyoub commented Sep 19, 2020 •
edited
Loading

@arw2019 I need a help here, why Linting step fail here?

@dsaxton dsaxton added the Testing pandas testing functions or related to the test suite label Sep 20, 2020
@dsaxton
Copy link
Member

dsaxton commented Sep 20, 2020 •
edited
Loading

So I don't think this is actually testable with the current setup. Apparently code run in tests is configured to raise an error instead of a warning on chained assignment (hence all the test failures):

pandas/pandas/conftest.py

Line 151 in d38dc06

pd.set_option("chained_assignment", "raise")

@samilAyoub
Copy link
Contributor Author

@dsaxton So we can modify the test to capture raising instead of warning when chained assignment is used.

@dsaxton
Copy link
Member

dsaxton commented Sep 20, 2020

@dsaxton So we can modify the test to capture raising instead of warning when chained assignment is used.

That seems reasonable

dsaxton
dsaxton reviewed Sep 20, 2020
View reviewed changes
@samilAyoub samilAyoub requested a review from dsaxton September 21, 2020 10:18
dsaxton
dsaxton approved these changes Sep 21, 2020
View reviewed changes
Copy link
Member

@dsaxton dsaxton left a comment

Choose a reason for hiding this comment

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

One tiny nitpick, otherwise looks good (CI failures are unrelated)

@dsaxton
Copy link
Member

dsaxton commented Sep 21, 2020

Also if you can now merge master that should fix the CI

@samilAyoub
Copy link
Contributor Author

@dsaxton did you mean merging master branch into this branch (add_test_subframe)?

@dsaxton
Copy link
Member

dsaxton commented Sep 21, 2020

@dsaxton did you mean merging master branch into this branch (add_test_subframe)?

Yes, actually it seems CI is fine so not really necessary

@samilAyoub
Copy link
Contributor Author

@dsaxton Thank you for your review !

jreback
jreback requested changes Sep 21, 2020
View reviewed changes
pandas/tests/indexing/test_loc.py Outdated
@@ -1130,3 +1131,17 @@ def test_loc_with_period_index_indexer():
tm.assert_frame_equal(df, df.loc[list(idx)])
tm.assert_frame_equal(df.iloc[0:5], df.loc[idx[0:5]])
tm.assert_frame_equal(df, df.loc[list(idx)])


Copy link
Contributor

Choose a reason for hiding this comment

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

can you put in pandas/tests/indexing/test_chaining_and_caching.py instead with the rest of these tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

Copy link
Contributor Author

@samilAyoub samilAyoub Sep 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

There is already a function called test_detect_chained_assignment_warnings that test if chained assignment raise a warning SettingWithCopyWarning in the case of warn option. We need just modify that function to detect also SettingWithCopyError in the case of raise option.

@jreback jreback added this to the 1.2 milestone Sep 21, 2020
@jreback jreback added the Indexing Related to indexing on series/frames, not to indexes themselves label Sep 21, 2020
@jreback jreback merged commit d9722ef into pandas-dev:master Sep 22, 2020
@jreback
Copy link
Contributor

jreback commented Sep 22, 2020

thanks @samilAyoub

kesmit13 pushed a commit to kesmit13/pandas that referenced this pull request Nov 2, 2020
@samilAyoub
add a test for loc method; check if a warning raise when replacing a … (
1abbe5d 
pandas-dev#36486)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arw2019 arw2019 arw2019 requested changes

@jreback jreback jreback approved these changes

@dsaxton dsaxton Awaiting requested review from dsaxton

Assignees
No one assigned
Labels
Indexing Related to indexing on series/frames, not to indexes themselves Testing pandas testing functions or related to the test suite
Projects
None yet
Milestone
1.2
Development

Successfully merging this pull request may close these issues.

QST: Replace subset of DataFrame by another subset of DataFrame
5 participants
@samilAyoub @pep8speaks @dsaxton @jreback @arw2019