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.

Sign up for GitHub

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

Jump to bottom

support for additional scipy nd interpolants #9599

Merged
merged 26 commits into from
Nov 6, 2024
Merged

support for additional scipy nd interpolants #9599

merged 26 commits into from
Nov 6, 2024

Conversation

hollymandel
Copy link
Contributor

@hollymandel hollymandel commented Oct 9, 2024 •
edited
Loading

  • Closes follow upstream scipy interpolation improvements #7704
  • Adds support for n dimensional tensor product interpolants slinear, cubic, quintic, and pchip.
  • Updates documentation of DataArray/Dataset.interp and DataArray/Dataset.interp_like to reflect updates to scipy interpolants called.
  • Adds argument reduce to allow disabling of reduction of interpolation along independent dimensions (see DataArray.interp() : poor performance #2223). Prior to this the behavior was always reduce=True. However for certain n-dimensional interpolants this will change the mathematical behavior of the interpolation (I think), so I have added an option for users to turn this off.

Outstanding uncertainty: dask/chunking behavior? Nothing about this PR touches the chunking of interpolation but I am a bit worried about this. Many interpolants, including those previously supported in one dimension, require nearby/all points from the original coordinate, and also don't parallelize well over the new coordinate. I'm not quite sure how this is currently being handled.

@hollymandel hollymandel force-pushed the issue_7704 branch 8 times, most recently from b8e4c50 to 34fb497 Compare October 11, 2024 19:09
@hollymandel hollymandel marked this pull request as ready for review October 11, 2024 19:14
@headtr1ck
Copy link
Collaborator

Thanks for this contribution.

Some remarks:

What is the minimal scipy version that is required for these new interpolators? Is it aligned with the minimum version for xarray or do we need some checks for that?

You have added several formatting changes to totally unrelated parts that make it more difficult to review then necessary. Please try to avoid that.

Doc build failed, have to check if random or related to your docstrings.

@hollymandel hollymandel force-pushed the issue_7704 branch 2 times, most recently from 4a26a7c to 34fb497 Compare October 14, 2024 15:18
@hollymandel
Copy link
Contributor Author

Thanks for the comments!

What is the minimal scipy version that is required for these new interpolators? Is it aligned with the minimum version for xarray or do we need some checks for that?

I'm seeing 1.11 in ci/requirements/min-all-deps.yml and these interpolants were added in 1.10 so I think we're good.

You have added several formatting changes to totally unrelated parts that make it more difficult to review then necessary. Please try to avoid that.

Oops, reverted, sorry!

@hollymandel hollymandel force-pushed the issue_7704 branch 2 times, most recently from 33f52ec to 211984d Compare October 14, 2024 15:49
@headtr1ck
Copy link
Collaborator

Also, I think the name "reduce" is not very intuitive, but I fail to come up with an alternative. "decompose" seems weird out of context. Does anyone have a suggestion?

@hollymandel hollymandel force-pushed the issue_7704 branch 2 times, most recently from cf8fa9f to 721695d Compare October 17, 2024 20:38
@hollymandel
Copy link
Contributor Author

Also, I think the name "reduce" is not very intuitive, but I fail to come up with an alternative. "decompose" seems weird out of context. Does anyone have a suggestion?

Perhaps the _interp makes decompose_interp sound less weird? Separate or split also sound OK to me.

dcherian
dcherian reviewed Oct 21, 2024
View reviewed changes
xarray/core/missing.py Outdated Show resolved Hide resolved
dcherian
dcherian reviewed Oct 21, 2024
View reviewed changes
xarray/core/dataarray.py Outdated Show resolved Hide resolved
dcherian
dcherian reviewed Oct 21, 2024
View reviewed changes
xarray/core/dataarray.py Outdated Show resolved Hide resolved
dcherian
dcherian reviewed Oct 21, 2024
View reviewed changes
xarray/core/dataarray.py Outdated Show resolved Hide resolved
dcherian
dcherian reviewed Oct 21, 2024
View reviewed changes
xarray/core/dataarray.py Outdated Show resolved Hide resolved
@dcherian
Copy link
Contributor

Many interpolants, including those previously supported in one dimension, require nearby/all points from the original coordinate, and also don't parallelize well over the new coordinate. I'm not quite sure how this is currently being handled.

I think we end up rechunking to a single in a very roundabout way. I don't think you need to be too concerned about it for this PR. It does need an overhaul though

@hollymandel
Copy link
Contributor Author

I think the failed test in ubuntu-latest py3.12 flaky is unrelated to this PR. I've rerun a few times and it keeps happening though, not sure how to move forward.

@keewis
Copy link
Collaborator

keewis commented Nov 4, 2024

that issue should be fixed by #9709, so the merge appears to have fixed that issue here, as well.

@headtr1ck headtr1ck added enhancement plan to merge Final call for comments labels Nov 6, 2024
@dcherian dcherian enabled auto-merge (squash) November 6, 2024 18:23
@dcherian dcherian disabled auto-merge November 6, 2024 19:13
@dcherian dcherian merged commit a26c816 into pydata:main Nov 6, 2024
27 of 29 checks passed
dcherian added a commit to dcherian/xarray that referenced this pull request Nov 7, 2024
@dcherian
Merge branch 'main' into groupby-shuffle
978fad9 
* main:
  Enforce ruff/flake8-pie rules (PIE) (pydata#9740)
  Enforce ruff/flake8-comprehensions rules (C4) (pydata#9724)
  Enforce ruff/Perflint rules (PERF)  (pydata#9730)
  Apply ruff rule RUF007 (pydata#9739)
  chmod -x (pydata#9725)
  Aplpy ruff rules (RUF) (pydata#9731)
  Fix typos found by codespell (pydata#9721)
  support for additional scipy nd interpolants  (pydata#9599)
  Apply ruff/flake8-simplify rules (SIM) (pydata#9727)
  Apply ruff/flake8-implicit-str-concat rules (ISC) (pydata#9722)
  Apply ruff/flake8-pie rules (PIE) (pydata#9726)
  Enforce ruff/pygrep-hooks rules (PGH) (pydata#9729)
  Move to micromamba 2 (pydata#9732)
  Fix groupby tests (pydata#9716)
  Add missing xarray.core.missing import (pydata#9714)
  Fix writing of DataTree subgroups to zarr or netCDF (pydata#9677)
  Bump pypa/gh-action-pypi-publish from 1.10.3 to 1.11.0 in the actions group (pydata#9707)
  Update pre-commit hooks (pydata#9713)
dcherian added a commit to scharlottej13/xarray that referenced this pull request Nov 9, 2024
@dcherian
Merge branch 'main' into dask-docs
67cc617 
* main: (125 commits)
  http:// → https:// (pydata#9748)
  Discard useless `!s` conversion in f-string (pydata#9752)
  Apply ruff/flake8-simplify rule SIM401 (pydata#9749)
  Use micromamba 1.5.10 where conda is needed (pydata#9737)
  pin array-api-strict<=2.1 (pydata#9751)
  Reorganise ruff rules (pydata#9738)
  use new conda-forge package pydap-server (pydata#9741)
  Enforce ruff/flake8-pie rules (PIE) (pydata#9740)
  Enforce ruff/flake8-comprehensions rules (C4) (pydata#9724)
  Enforce ruff/Perflint rules (PERF)  (pydata#9730)
  Apply ruff rule RUF007 (pydata#9739)
  chmod -x (pydata#9725)
  Aplpy ruff rules (RUF) (pydata#9731)
  Fix typos found by codespell (pydata#9721)
  support for additional scipy nd interpolants  (pydata#9599)
  Apply ruff/flake8-simplify rules (SIM) (pydata#9727)
  Apply ruff/flake8-implicit-str-concat rules (ISC) (pydata#9722)
  Apply ruff/flake8-pie rules (PIE) (pydata#9726)
  Enforce ruff/pygrep-hooks rules (PGH) (pydata#9729)
  Move to micromamba 2 (pydata#9732)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcherian dcherian dcherian left review comments

Assignees
No one assigned
Labels
enhancement plan to merge Final call for comments
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

follow upstream scipy interpolation improvements
4 participants
@hollymandel @headtr1ck @dcherian @keewis