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
Contributor

@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
Contributor

@arw2019 arw2019 Sep 19, 2020

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

@samilAyoub samilAyoub Sep 19, 2020

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
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
Contributor

@arw2019 arw2019 Sep 19, 2020

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

@samilAyoub samilAyoub Sep 19, 2020

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
dsaxton reviewed Sep 20, 2020
View reviewed changes
dsaxton
dsaxton reviewed Sep 20, 2020
View reviewed changes
dsaxton
dsaxton reviewed Sep 20, 2020
View reviewed changes
@dsaxton
Copy link
Contributor

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

samilAyoub commented Sep 20, 2020

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

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
Contributor

@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
Contributor

dsaxton commented Sep 21, 2020

Also if you can now merge master that should fix the CI
@samilAyoub @dsaxton
Update pandas/tests/indexing/test_loc.py
1708a9e 
Co-authored-by: Daniel Saxton <2658661+dsaxton@users.noreply.github.com>
@samilAyoub
Copy link
Contributor Author

samilAyoub commented Sep 21, 2020

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

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

samilAyoub commented Sep 21, 2020

@dsaxton Thank you for your review !
jreback
jreback requested changes Sep 21, 2020
View reviewed changes
pandas/tests/indexing/test_loc.py Outdated
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

@jreback jreback Sep 21, 2020

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

@samilAyoub samilAyoub Sep 22, 2020

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

Labels

Indexing Related to indexing on series/frames, not to indexes themselves Testing pandas testing functions or related to the test suite

5 participants

@samilAyoub @pep8speaks @dsaxton @jreback @arw2019