meggart / DiskArrays.jl

Other
72 stars 13 forks source link

Indxing type stability #137

Open rafaqz opened 7 months ago

rafaqz commented 7 months ago

We need to thoroughly audit type stability when indexing, maybe with some @inferred tests.

See: https://github.com/meggart/DiskArrays.jl/issues/131

Alexander-Barth commented 7 months ago

I think it would be good to avoid this if statement here using dispatch:

https://github.com/meggart/DiskArrays.jl/blob/ada19c131c92c710d81d728c84e6b947b3004a83/src/diskarray.jl#L35

meggart commented 6 months ago

From my tests, everything should be type stable now with #146 . However, we don't have formal unit tests for this yet. @rafaqz I have never written type-stability tests, do you have a few pointers?

@Alexander-Barth I know it would be great to get rid of any runtime checks. However, some things are only detectable at runtime, for example if a vector index like [7,5,6,4,1,2,3] is dense or not and can be replaced by a single unit range or has to be split up. However, we have organized the code so that all return types should definitely be infered.

rafaqz commented 6 months ago

There are a few ways. @test @inferred can help but wont always pick up internal type stability problems that never escape the function. Checking @test (@allocations f(x)) == 0 can help with that, and maybe using something larger than zero if you know how many allocations there should be. Type instability allocates a lot.

rafaqz commented 6 months ago

Theres also @test_call in JET but it often returns too many things to be useful

https://aviatesk.github.io/JET.jl/dev/jetanalysis/#JET.@test_call

meggart commented 6 months ago

For now I added some @test @inferred tests in the unit tests to make sure that type instabilities never leak out of the functions and this already helped detecting a few branches that were still type-unstable. I would suggest that more testing with JET can be added later when problems arise.