CLN: simplify interpolate_2d and callers #36624
Conversation
| 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.
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.
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.
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.
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.
- it may be that we can disallow fill_value at the top of Series.interpolate. making
**kwargsexplicit 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.
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.
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.
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 |
jreback
added
API - Consistency
4 participants
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diff