-
-
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
Conversation
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 comment otherwise lgtm
@@ -1181,12 +1181,15 @@ def interpolate( | |||
m = None | |||
|
|||
if m is not None: | |||
if fill_value is not None: | |||
# similar to validate_fillna_kwargs | |||
raise ValueError("Cannot pass both fill_value and method") |
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?
|
||
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
do we really need the ValueError if fill_value is passed?
In master:
>>> ser = pd.Series([1, 2, np.nan, 4])
>>> ser.interpolate(method="pad", fill_value=99)
0 1.0
1 2.0
2 NaN
3 4.0
dtype: float64
I'm pretty confident this isn't intentional.
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.
but on master and this PR
>>> pd.__version__
'1.2.0.dev0+499.g3b12293cea'
>>> ser = pd.Series([1, 2, np.nan, 4])
>>> ser.interpolate(fill_value=99)
0 1.0
1 2.0
2 3.0
3 4.0
dtype: float64
>>> ser.interpolate(humpty_dumpty=99)
0 1.0
1 2.0
2 3.0
3 4.0
dtype: float64
why the ValueError when method passed?
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.
- it may be that we can disallow fill_value at the top of Series.interpolate. making
**kwargs
explicit would be nice. But i haven't thoroughly checked if thats possible. - BlockManager.interpolate also gets called from fillna, so we need to rule out fill_value at the place where this PR does 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.
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 .interpolate()
will raise (or can do it after when all non-named keyword args are banned.
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.
so a ValueError may be appropriate for fill_na, but the test is for Series.interpolate. Why not TypeError: interpolate() got an unexpected keyword argument 'fill_value'
?
|
||
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 comment
The 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 .interpolate()
will raise (or can do it after when all non-named keyword args are banned.
yah lets save it for the end |
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff