xarray icon indicating copy to clipboard operation
xarray copied to clipboard

support for additional scipy nd interpolants

Open hollymandel opened this issue 1 year ago • 5 comments

  • [x] Closes #7704
  • [x] Adds support for n dimensional tensor product interpolants slinear, cubic, quintic, and pchip.
  • [x] Updates documentation of DataArray/Dataset.interp and DataArray/Dataset.interp_like to reflect updates to scipy interpolants called.
  • [x] Adds argument reduce to allow disabling of reduction of interpolation along independent dimensions (see #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 avatar Oct 09 '24 18:10 hollymandel

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.

headtr1ck avatar Oct 13 '24 17:10 headtr1ck

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 avatar Oct 14 '24 15:10 hollymandel

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?

headtr1ck avatar Oct 14 '24 16:10 headtr1ck

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.

hollymandel avatar Oct 17 '24 20:10 hollymandel

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

dcherian avatar Oct 21 '24 21:10 dcherian

Thanks @hollymandel this is looking quite close. I only have a few small refinement requests.

dcherian avatar Oct 23 '24 17:10 dcherian

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.

hollymandel avatar Nov 04 '24 03:11 hollymandel

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

keewis avatar Nov 04 '24 09:11 keewis