sisl / GridInterpolations.jl

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

Attempt to improve type stability #47

Closed ctessum closed 1 month ago

ctessum commented 2 months ago

Fixes #38

mykelk commented 2 months ago

Thanks for the PR! It looks like some of the tests are not passing?

ctessum commented 2 months ago

I'll try to fix it. I will also add some additional tests.

ctessum commented 2 months ago

OK, the tests are passing now. I also attempted to reduce the allocations in the interpolants function, and marked the remaining allocations with a @test_broken. Let me know what you think.

mykelk commented 2 months ago

It seems to break on Julia 1.0. We don't necessarily have to provide backward support to 1.0, but it might be nice if it is not too complicated.

ERROR: LoadError: LoadError: UndefVarError: @allocations not defined
Stacktrace:
 [1] top-level scope
 [2] include at ./boot.jl:317 [inlined]
 [3] include_relative(::Module, ::String) at ./loading.jl:1044
 [4] include(::Module, ::String) at ./sysimg.jl:29
 [5] include(::String) at ./client.jl:392
 [6] top-level scope at none:0
in expression starting at /home/runner/work/GridInterpolations.jl/GridInterpolations.jl/test/runtests.jl:417
ctessum commented 2 months ago

Sure, I think just removing the test for allocations might fix that. (I don't have julia 1.0 installed to check) Does it work now?

mykelk commented 2 months ago

Got it. I think we can merge this after @zsunberg gives us a thumbs up!

mykelk commented 1 month ago

Cool. I think we can merge once @zsunberg concerns are addressed in this PR.

zsunberg commented 1 month ago

@ctessum Thanks again!

One note for the future: this PR was a bit hard to read because it included both substantive changes to the code and a bunch of minor style changes (e.g. adding spaces). The style changes are nice, but it is better to make separate PRs, just so that it is easy to see and deal with the important changes :smile:

zsunberg commented 1 month ago

BTW I tried to add allocation tests in #50, but they are failing :/ It seems like we didn't quite prevent all the allocations. The results were the same before this PR though.

ctessum commented 1 month ago

Sorry about the style changes, they were just added automatically and I didn't notice until later.