rafaqz / DimensionalData.jl

Named dimensions and indexing for julia arrays and other data
https://rafaqz.github.io/DimensionalData.jl/stable/
MIT License
274 stars 39 forks source link

linear interpolation #609

Closed DanDeepPhase closed 3 months ago

DanDeepPhase commented 8 months ago

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().
rafaqz commented 8 months ago

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.

DanDeepPhase commented 8 months ago

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.

rafaqz commented 7 months ago

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

codecov[bot] commented 7 months ago

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 ```diff @@ 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.

asinghvi17 commented 3 months ago

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.

rafaqz commented 3 months ago

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