Ind2x mvec

[mattuntergassmair]

It seems that pre-specifying the vector size using MVector from StaticArrays helps to significantly improve the performance of ind2x

[mattuntergassmair]

Could potentially be a breaking change if users were relying on the return type of ind2x to be Array rather than AbstractArray

[coveralls]

Coverage decreased (-0.07%) to 95.726% when pulling 16403a7 on mattuntergassmair:ind2x_mvec into 5dcb458 on sisl:master.

[mykelk]

@MaximeBouton shall we merge this?

[zsunberg]

I like using StaticArrays! Shouldn't you be able to get it down to 0 allocs though? You have only reduced it to 1000000 from 2000000

[MaximeBouton]

if we want to reduce allocation to zero, adding a container for x in grid might do it. @zsunberg @mattuntergassmair

[mattuntergassmair]

You mean adding a MVector{D, Float64} as a field to the RectangleGrid struct, so we can simply use that to store the result instead of allocating - did I understand the suggestion correctly?
It would help with the allocations, but I think what would happen is that if you call ind2x() twice, and you are still holding on to the result computed in the first call, then it gets overwritten in the second call without the user noticing, right?

[mattuntergassmair]

I guess in that case users should just pre-allocate memory on their side and use ind2x!() rather than ind2x()

[zsunberg]

x is typically pretty low-dimensional, right? I think ind2x should return an SVector. I think that should get rid of all the allocations and save a large amount of time.

In fact, I think the calls between ind2x and ind2x! should be switched, i.e. ind2x should contain the logic, and ind2x! should be implemented as

function ind2x!(grid, ind, x)
x[:] = ind2x(grid, ind)
end

[MaximeBouton]

why do you need the type annotation here? I would check with @code_warntype that this is type stable.

[mattuntergassmair]

I just kept the type annotation that was already present in the form of ::Array{Float64}, but should not be necessarry, agreed. My understanding was that it's more explicit than required, but also doesn't harm, right?

[mattuntergassmair]

I like using StaticArrays! Shouldn't you be able to get it down to 0 allocs though? You have only reduced it to 1000000 from 2000000

As for the allocations, I'm running the function benchmark_ind2x() with default parameters n_dims = 6, n_points_per_dim = 10, leading to 10^6 points. The benchmark function then calls ind2x() 10^6=1000000 times. So I think the allocs are as expected (as opposed to ind2x!() which should be non-allocating). I think what was happening earlier was that there was a copy somewhere leading to 2 x 10^6 allocs.

[zsunberg]

Could potentially be a breaking change if users were relying on the return type of ind2x to be Array rather than AbstractArray

I think this is fine. If people are relying on a concrete array type, I will not feel sorry if their code gets broken :)

[MaximeBouton]

Shouldn't we expect a similar speed up by replacing those:
https://github.com/sisl/GridInterpolations.jl/blob/master/src/GridInterpolations.jl#L36
by mutable statically sized arrays?

[mattuntergassmair]

Shouldn't we expect a similar speed up by replacing those:
https://github.com/sisl/GridInterpolations.jl/blob/master/src/GridInterpolations.jl#L36
by mutable statically sized arrays?

True, we could probly fix the size of most vectors that are part of the RectangleGrid struct and that should help with allocation and performance.
I could look into it but might need some help since I don't feel I understand the code base sufficiently well. Anyone want to team up on this (for a different PR)?

[zsunberg]

Those allocations are in the constructor, so I don't think it will make too much difference because they are only done once when the object is constructed.

…

On Fri, Mar 6, 2020 at 4:29 PM Maxime ***@***.***> wrote: Shouldn't we expect a similar speed up by replacing those: https://github.com/sisl/GridInterpolations.jl/blob/master/src/GridInterpolations.jl#L36 by mutable statically sized arrays? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#32?email_source=notifications&email_token=ABALI26NZBQ4T2K4OVNV5OTRGGBODA5CNFSM4LCUEXH2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEODFVIY#issuecomment-596007587>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABALI27K3ONJYOXWJJKLJTLRGGBODANCNFSM4LCUEXHQ> .

[mattuntergassmair]

Those allocations are in the constructor, so I don't think it will make too much difference because they are only done once when the object is constructed.
…
On Fri, Mar 6, 2020 at 4:29 PM Maxime @.***> wrote: Shouldn't we expect a similar speed up by replacing those: https://github.com/sisl/GridInterpolations.jl/blob/master/src/GridInterpolations.jl#L36 by mutable statically sized arrays? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#32?email_source=notifications&email_token=ABALI26NZBQ4T2K4OVNV5OTRGGBODA5CNFSM4LCUEXH2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEODFVIY#issuecomment-596007587>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABALI27K3ONJYOXWJJKLJTLRGGBODANCNFSM4LCUEXHQ .

Agreed that it may not make a difference in the constructor itself, but we could make the member variables static in size, maybe the fixed size helps with memory optimization. I can give it a shot and report back.

[zsunberg]

As for the allocations, I'm running the function benchmark_ind2x() with default parameters n_dims = 6, n_points_per_dim = 10, leading to 10^6 points. The benchmark function then calls ind2x() 10^6=1000000 times. So I think the allocs are as expected (as opposed to ind2x!() which should be non-allocating). I think what was happening earlier was that there was a copy somewhere leading to 2 x 10^6 allocs.

@MVector zeros(D) Allocates. We should really do this with SVectors. Right now we are getting a 2x speedup with mvectors. I bet we will get a 10 or 100x speedup with svectors

[MaximeBouton]

For a broader utilization of MVector for the interpolation code, one thing to keep in mind is that if the size is larger than 100 we should expect decrease in performance according to the StaticArrays.jl readme. Since the size is given by 2^num_dimensions, it grows pretty fast.

[mykelk]

What is the status of this PR?