Skip to content
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

Fix pandas' interpolate(fill_value=) error #8139

Merged
merged 1 commit into from
Sep 4, 2023

Conversation

max-sixty
Copy link
Collaborator

@max-sixty max-sixty commented Sep 2, 2023

Pandas no longer has a fill_value parameter for interpolate.

Weirdly I wasn't getting this locally, on pandas 2.1.0, only in CI on https://github.com/pydata/xarray/actions/runs/6054400455/job/16431747966?pr=8138.

Removing it passes locally, let's see whether this works in CI

Would close #8125

Pandas no longer has a `fill_value` parameter for `interpolate`.

Weirdly I wasn't getting this locally, on pandas 2.1.0, only in CI on https://github.com/pydata/xarray/actions/runs/6054400455/job/16431747966?pr=8138.

Removing it passes locally, let's see whether this works in CI
@max-sixty max-sixty changed the title Fix pandas interpolate(fill_value=) error Fix pandas' interpolate(fill_value=) error Sep 2, 2023
@max-sixty
Copy link
Collaborator Author

This indeed fixes the errors. I've been away for a while so don't want to self-merge quite yet, but will if no one comments otherwise in the next day or so...

@kmuehlbauer
Copy link
Contributor

@max-sixty This is a regression bug in pandas, see pandas-dev/pandas#54920.

fill_value is still a kwarg for the scipy based interpolators. Only for the fill_methods this is an issue.

@max-sixty
Copy link
Collaborator Author

OK — though does it not being required for the tests to pass suggest we can merge it regardless?

@kmuehlbauer
Copy link
Contributor

The tests pass because np.nan is the default fill_value in the scipy interpolators and maybe because fill_value is actually not needed for these specific tests. That would indicate, that the tests would need adaption to take fill_value correctly into account as needed for the scipy based interpolation methods.

Please have a look here at the scipy docs https://docs.scipy.org/doc/scipy/reference/generated/scipy.interpolate.LinearNDInterpolator.html#scipy.interpolate.LinearNDInterpolator

_Value used to fill in for requested points outside of the convex hull of the input points. If not provided, then the default is nan.

@max-sixty
Copy link
Collaborator Author

max-sixty commented Sep 2, 2023

Right — agree the argument isn't doing anything in these tests. Is there a good reason to keep it vs. merging to fix the tests?

I also agree that we could have different tests which test for fill_value if that's important to test. (though would be an additional PR)

@kmuehlbauer
Copy link
Contributor

kmuehlbauer commented Sep 4, 2023

@max-sixty Although upstream has milestoned this issue for the next release it might still take some time. Maybe it's easiest to move forward here and open an issue to create proper tests which take fill_value into account (optional PR)?

@max-sixty max-sixty merged commit e9c1962 into pydata:main Sep 4, 2023
25 of 28 checks passed
@max-sixty max-sixty deleted the pandas-interpolate branch September 28, 2023 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

failing tests with pandas 2.1
2 participants