-
-
Notifications
You must be signed in to change notification settings - Fork 18.6k
CLN: simplify interpolate_2d and callers #36624
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -340,6 +340,14 @@ def test_interp_invalid_method(self, invalid_method): | |
with pytest.raises(ValueError, match=msg): | ||
s.interpolate(method=invalid_method, limit=-1) | ||
|
||
def test_interp_invalid_method_and_value(self): | ||
# GH#36624 | ||
ser = Series([1, 3, np.nan, 12, np.nan, 25]) | ||
|
||
msg = "Cannot pass both fill_value and method" | ||
with pytest.raises(ValueError, match=msg): | ||
ser.interpolate(fill_value=3, method="pad") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fill_value is not in the Series.interpolate api. This is accepted through kwargs from the public api? Then fill_value is accepted and passed along internally? I assume none of the scipy interpolate functions accept fill_value? or won't in the future! do we really need the ValueError if fill_value is passed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In master:
I'm pretty confident this isn't intentional. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but on master and this PR
why the ValueError when method passed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this looks ok to me; i think we are accepting too much here accidently. @jbrockmendel can you add a whatsnew note in any case (just saying passing invalid keyword args to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so a ValueError may be appropriate for fill_na, but the test is for Series.interpolate. Why not |
||
|
||
def test_interp_limit_forward(self): | ||
s = Series([1, 3, np.nan, np.nan, np.nan, 11]) | ||
|
||
|
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 a test for codecov?