search

sisl / GridInterpolations.jl

Multidimensional grid interpolation in arbitrary dimensions
Other
51 stars 12 forks source link

Redundant and allocating method for matrixes #48

Closed ctessum closed 2 months ago

ctessum commented 3 months ago

Here: https://github.com/sisl/GridInterpolations.jl/blob/a955ae2ff6ab74491daa7ce72c2bb401e289d137/src/GridInterpolations.jl#L142

Because: 

julia> Matrix
Matrix (alias for Array{T, 2} where T)

and there is already a method for Array, it seems like this method isn't needed, so could be deleted, and it reallocates the entire array, so should be avoided. But maybe I misunderstand something?

mykelk commented 3 months ago

Good question. Any thoughts @zsunberg ?

zsunberg commented 2 months ago

After looking at the blame, the best I can tell is that it was meant to flatten the matrix into a vector, but I don't think that would make a difference in the result. I think we could remove it.

I wish we had up-to-date code coverage set up, but according to the latest coverage result from 2021, it is a covered line, so it should be safe to remove if the tests pass :laughing: .

mykelk commented 2 months ago

Sounds good. It looks like it is part of #47 !

ctessum commented 2 months ago

I believe the overall effect of this method is to convert any type of matrix to a float64 matrix before doing interpolation. However, with #47 interpolation should work natively for non-float64 matrixes, so this matrix method shouldn't be needed anymore.

zsunberg commented 2 months ago

This was fixed as part of #47 I believe