DimensionalData.jl icon indicating copy to clipboard operation
DimensionalData.jl copied to clipboard

linear interpolation

Open DanDeepPhase opened this issue 1 year ago • 4 comments

This implements a general linear interpolation. Summary:

  1. updated module name
  2. i went with the "create an interpolator" route, and started with linear_interpolation
  3. You had been implementing a regridding algorithm along some but not all dimensions. It wasn't working when i cloned, and i wasn't able to get it to work, as detailed below
  4. its a start, and Interpolations.interpolate() is probably a better function to extend, which should be easy enough. It requires returning the values of the nodes to avoid recursion, which i couldn't remember how to do.

I commented out your existing code because it was failing for me in a few ways. I don't really understand extensions, so could be easy fixes.

  1. Can you use extra dependencies (rasters) in the extension?
  2. Does the extension need a TOML at that point?was getting an explicit call for DimensionalData problem which i think was in the const DD =DimensionalData
  3. when i try to run test code on the extension, the only functions that work for me are the ones that are extended. so i can run Interpolations.linear_interpolation() but not interp().

DanDeepPhase avatar Feb 02 '24 17:02 DanDeepPhase

Yeah that's how it works, you can only add to function defined somewhere else, you can't "see" functions or types created inside the extension.

Returning an interpolator is probably a better strategy too.

rafaqz avatar Feb 02 '24 20:02 rafaqz

I trimmed it down a lot. The code is pretty straightforward now. Perhaps it should extend interpolate, but i tend to use the convenience functions more.

For future effort, i could see two nice to have functions:

  1. interp_slice(A,dim(val)) = create a slice of a DimArray at an off-index value. I think this is what your interp code was doing. I like the ability to drop "referenced dimensions", like A[:,:,1]
  2. regrid(A, dim(vector), dim2(vector2)) pass an array of coordinates, and interpolate to those values

not super good with CI, seemed like a compat issue, but it didn't resolve when i added interpolations to the compat list.

DanDeepPhase avatar Feb 05 '24 15:02 DanDeepPhase

Sorry was too busy with other things. The Project.toml file is broken, you need an equals sign for the Interpolations version here:

https://github.com/rafaqz/DimensionalData.jl/actions/runs/7786303160/job/21230819886#step:4:46

rafaqz avatar Feb 29 '24 08:02 rafaqz

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes are missing coverage. Please review.

:exclamation: No coverage uploaded for pull request base (interpolations@06b5cd7). Click here to learn what that means.

Files Patch % Lines
ext/DimensionalDataInterpolations.jl 0.00% 4 Missing :warning:
Additional details and impacted files
@@                Coverage Diff                @@
##             interpolations     #609   +/-   ##
=================================================
  Coverage                  ?   87.13%           
=================================================
  Files                     ?       44           
  Lines                     ?     3450           
  Branches                  ?        0           
=================================================
  Hits                      ?     3006           
  Misses                    ?      444           
  Partials                  ?        0           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Feb 29 '24 20:02 codecov[bot]

I wonder if this could also use the DimensionalData Makie utility functions...it seems like one would want to shift indices to sample at the center and similar things when interpolating by default.

I do wish that Interpolations.jl respected the array axis interface, but I guess that would be a pretty annoying thing to do and would have to be upstream in any case. That being said, for full compatibility we should also be overriding like:

Interpolations.interpolate(A::AbstractDimArray, interpmode; kw...) = Interpolations.interpolate(DimensionalData.index(dims(A)), A; kw...)

or something, to provide general support.

asinghvi17 avatar Jun 24 '24 13:06 asinghvi17

Just realised this PR was against the other PR, so I'll reopen it

rafaqz avatar Jun 24 '24 16:06 rafaqz