Generalise _interpolate! to allow mixed dimensions#33
Conversation
rafaqz
changed the title
_interpolate! to allow mixed dimensions_interpolate! to allow mixed dimensions
SouthEndMusic
left a comment
SouthEndMusic
left a comment
There was a problem hiding this comment.
I don't fully onderstand everything yet, but overall it looks good. I added some comments.
|
FYI there is a lot left to do, the tests that pass are because there are no missing dimensions but this will break if there are any. |
|
This should be working now. |
|
There are a few outstanding questions not - like #38 on what dimensions the weights matrix should have now. But more importantly, what should Currently Please chime in here this should be finalised and tested in this PR. |
| for (t₁, t₂) in zip(tᵢ, tᵢ₊₁) | ||
| t_vol *= t₂ - t₁ | ||
| out, valid_derivative_orders = check_derivative_order(interp_dims, derivative_orders, ts, out) | ||
| valid_derivative_orders || return out |
There was a problem hiding this comment.
It should be more clear that !valid_derivative_orders does not mean that the input is invalid but that it is certain that the output is 0.
There was a problem hiding this comment.
What do you suggest
| out::Union{Number, AbstractArray{<:Any, N_out}}, | ||
| A::NDInterpolation{N, N_in, N_out}, | ||
| ts::Tuple{Vararg{Any, N}}, | ||
| idx::Tuple{Vararg{Any ,N}}, |
There was a problem hiding this comment.
Why allow the index to be any type?
There was a problem hiding this comment.
It can be Colon() etc for NoInterpolationDimension.
Although this could all be cleaned up a bit.
Why not support multiple approaches? Have one method which calls |
|
Maybe, I was leaning to choosing one or the other because mixed tuple lengths both accepted in the same function may invite some user error and confusion. |
|
I am still in favor of supporting multiple options if reasonably possible. We could make the difference more explicit by using different functions or pass an optional Boolean flag. Then we still have to decide what the default behavior is. From a mathematical perspective I would prefer the current approach on main, but if passing some trivial object for the non-interpolated dimensions makes more sense from a code perspective, that's fine too. |
|
A bool keyword on a tuple length isn't necessarily type stable unless it reliably constant props, and I would prefer simplicity over options, so making a choice either way seems better to me. Given this difference of perspective its probably better discussed in a new PR and not hold up this one, so I kept the behavior as it was on main. |
|
This seems good to go to me. I don't think its even a breaking change? I have some performance improvements too but will PR those when this lands. (Please use squash if/when this is merged, there are a lot of small commits not intended for the history) |
|
@rafaqz sorry for the long time until my response. Do you still want to merge this? It looks like some conflicts have accumulated. |
SouthEndMusic
left a comment
SouthEndMusic
left a comment
There was a problem hiding this comment.
I left a few small comments, and there are some TODO's left. It looks like you know what you are doing. Please go over the docstrings and the README/documentation before merging.
| ## Arguments | ||
|
|
||
| - `interp_dims`: A tuple of identically typed interpolation dimensions. | ||
| - `u`: The array to be interpolated. |
There was a problem hiding this comment.
The NDinterpolation docstring needs an update
| # We need to add the NoInterpolationDimensions for all arguments to _interpolate. | ||
| # Currently interp() accepts only the interpolating dimensions. | ||
| # We could switch this so it needs indices for NoInterpolationDimension, but thats a breaking change. |
There was a problem hiding this comment.
I vaguely remember the discussion on this. I think avoiding the breaking change would be nicest. When indexing for the NoInterpolationDimension we should support integers and slices. We could overload getindex for this, or is that too much outside the scope of that pattern/syntax?
| stencil(::NoInterpolationDimension) = 1 | ||
| stencil(d::BSplineInterpolationDimension) = 1:(d.degree + 1) | ||
|
|
||
| # Precalculate coefficient/s |
There was a problem hiding this comment.
| # Precalculate coefficient/s | |
| # Precalculate coefficients |
| t::Tuple{Vararg{Number, N_in}}, | ||
| idx::NTuple{N_in, <:Integer}, | ||
| derivative_orders::NTuple{N_in, <:Integer}, | ||
| Base.@propagate_inbounds function _interpolate!( |
There was a problem hiding this comment.
I feel like this main function belongs either in a separate file or in DataInterpolationsND.jl
2 participants
This PR is an exploration of #31 and #32 to see whats possible.
Probably quite slow in current form but this largely works already for the current tests.The idea is to break each dimension into a preparation step, a definition of the iteration space, and then scaling and index generation functions for the inner loop, then call all of these function from the same shared
_interpolate!method.Will need to add some tests for mixed dimension types.Closes #31 and closes #32