-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: adding numpy errstate to dataframe/series broadcasting (GH16378) #16433
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.
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
BUG: adding numpy errstate to dataframe/series broadcasting (GH16378) #16433
Conversation
Looks like this may also close #16306 - could you add some confirming tests? |
df = DataFrame({'A': [np.nan, 2.0, np.nan]}) | ||
s = Series([1, 1, 1]) | ||
|
||
df.clip_lower(s, axis=0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can use with tm.assert_produces_warning(None):
around this to assert no warning raised rather than reading stderr
df = DataFrame({'A': [np.nan, 2.0, np.nan]}) | ||
s = Series([1, 1, 1]) | ||
|
||
df.clip_lower(s, axis=0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you write this as
with warnings.catch_warnings(record=True) as w:
df.clip_lower(s, axis=0)
assert len(w) == 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And you shouldnt need the capsys
then (may have to import warnings).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvm, do what @chris-b1 said, that's shorter than catching and asserting 😄
@@ -229,3 +229,12 @@ def test_to_records_datetimeindex_with_tz(self, tz): | |||
|
|||
# both converted to UTC, so they are equal | |||
tm.assert_numpy_array_equal(result, expected) | |||
|
|||
def test_to_series(self, capsys): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move this to pandas/tests/frame/test_analytics.py
Codecov Report
@@ Coverage Diff @@
## master #16433 +/- ##
==========================================
- Coverage 90.42% 90.42% -0.01%
==========================================
Files 161 161
Lines 51023 51024 +1
==========================================
Hits 46138 46138
- Misses 4885 4886 +1
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #16433 +/- ##
==========================================
+ Coverage 90.42% 90.42% +<.01%
==========================================
Files 161 161
Lines 51027 51028 +1
==========================================
+ Hits 46142 46143 +1
Misses 4885 4885
Continue to review full report at Codecov.
|
doc/source/whatsnew/v0.20.2.txt
Outdated
@@ -42,6 +42,7 @@ Conversion | |||
^^^^^^^^^^ | |||
|
|||
- Bug in ``pd.to_numeric()`` in which empty data inputs were causing Python to crash (:issue:`16302`) | |||
- Bug when broadcasting dataframe to series silencing numpy warning messages in common use case (:issue:`16378`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Silence numpy warnings when broadcasting DataFrame to Series with comparison ops.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add 16306 as well
pandas/tests/frame/test_analytics.py
Outdated
@@ -2073,3 +2073,14 @@ def test_n_duplicate_index(self, df_duplicates, n, order): | |||
result = df.nlargest(n, order) | |||
expected = df.sort_values(order, ascending=False).head(n) | |||
tm.assert_frame_equal(result, expected) | |||
|
|||
def test_series_broadcasting(self): | |||
df = DataFrame({'A': [np.nan, 2.0, np.nan]}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add the issue number as a comment here. (and a 1-line comment that this is a smoke test for numpy warnings)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor doc comments. ping on green.
c72dc40
to
fe1ab74
Compare
@andrewarcher small lint failure here: https://travis-ci.org/pandas-dev/pandas/jobs/235018875#L1380 Mind removing the extra newline at the end of the file? Then we can get it merged. Actually I'll just push a commit fixing that quick. I'm guessing your editor may be setup to add them automatically, and I don't want you to have to fiddle with settings. |
pandas/tests/frame/test_analytics.py
Outdated
|
||
with tm.assert_produces_warning(None): | ||
df.clip_lower(s, axis=0) | ||
df.gt(s, axis=0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pd.DataFrame([0,0]).gt(pd.Series([0, np.nan]), axis=0)
can you also add this test (the Series has the nan)
maybe just loop over these e.g.
for op in ['lt', 'le', 'ge', 'lt', 'eq', 'ne']:
getattr(df, op)(s, axis=0)
with tm.assert_produces_warning(None): | ||
df_nan.clip_lower(s, axis=0) | ||
for op in ['lt', 'le', 'gt', 'ge', 'eq', 'ne']: | ||
getattr(df, op)(s_nan, axis=0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hah, now we'll have opposite problem.
pandas/tests/frame/test_analytics.py:2088:47: W292 no newline at end of file
Basically line 2088 should end with a newline, but there shouldn't be a line 2089 that's just a newline. Want me to push a fix for this, or do you want to take care of it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I updated it correctly this time - if not, could you please push the correct fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that looks good.
…h comparison ops (GH16378, GH16306) TST: test for fix of GH16378, GH16306
3d41393
to
c78ee39
Compare
Merging (the Appveyor failure was fixed in master this morning). Thanks @andrewarcher! |
…, GH16306) (pandas-dev#16433) TST: test for fix of GH16378, GH16306(cherry picked from commit 96f3e7c)
…, GH16306) (pandas-dev#16433) TST: test for fix of GH16378, GH16306
closes #16378
closes #16306