Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redundant and allocating method for matrixes #48

Closed
ctessum opened this issue Jul 5, 2024 · 5 comments · Fixed by #49
Closed

Redundant and allocating method for matrixes #48

ctessum opened this issue Jul 5, 2024 · 5 comments · Fixed by #49

Comments

@ctessum
Copy link
Contributor

ctessum commented Jul 5, 2024

Here:

interpolate(grid::AbstractGrid, data::Matrix, x::AbstractVector) = interpolate(grid, map(Float64, data[:]), x)

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
Copy link
Member

mykelk commented Jul 6, 2024

Good question. Any thoughts @zsunberg ?

@zsunberg
Copy link
Member

zsunberg commented Jul 8, 2024

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 😆 .

@mykelk
Copy link
Member

mykelk commented Jul 10, 2024

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

ctessum added a commit to ctessum/GridInterpolations.jl that referenced this issue Jul 11, 2024
@ctessum
Copy link
Contributor Author

ctessum commented Jul 11, 2024

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
Copy link
Member

This was fixed as part of #47 I believe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants