search

JuliaArrays / AxisArrays.jl

Performant arrays where each dimension can have a named axis with values
http://JuliaArrays.github.io/AxisArrays.jl/latest/
Other
200 stars 41 forks source link

conversion and copy of arrays which differ by axis permutations #108

Closed mdavezac closed 2 years ago

mdavezac commented 7 years ago

This PR addresses the problem of converting or copying between arrays that address essentially the same data, but with different in-memory order. In other words, when two arrays share the same axis but in different order axisnames(A) == (:a, :b, :c) vs axisnames(B) == (:b, :a, :c).

This can be useful for instance when going from array of structures to structure of arrays idioms.

ajkeller34 commented 7 years ago

Since this touches core.jl maybe I'll ping @timholy for another opinion, but otherwise these seem like nice improvements. LGTM after my comments are addressed. Thanks for the PR.

mdavezac commented 7 years ago

I've updated the code so that equivalent axes are identified strictly by name. If I understand things correctly, that should take care of your comments, @ajkeller34.

mdavezac commented 7 years ago

I think you are right about the behaviour with respect to size. copy!'s behaviour for standard arrays is richer than I knew. Guess I should have looked before submitting the PR 😊

I've added an ignore_axes as suggested and made the efficiency improvement you pointed out. I've also added copy!(dest, (sub-block...), src, (sub-block...)) to the mix. However, it isn't as clear how we would want to ignore axes information this time around or whether it's needed or desirable, so I have not implemented a ignore_axes argument for it.

More specifically, we could ignore axis information both when (a) identifying the sub-blocks and when (b) copying. But that's already implemented as is. Or we could ignore information only in (b), but that would mean the order of the axis in indices tuples matter, and hence we would need to recover the given permutation. A bit painful and somewhat non-intuitive.

Thanks for taking the time and iterating this. As long as it converges asymptotically towards better code, it's all for the best.

mdavezac commented 6 years ago

Any news?

timholy commented 6 years ago

This is another example of potential confusion in treating an AxisArray as both a "standard" rectangular container and as one indexed by Axis objects. Overall I lean towards the behavior implemented here.

I'm wondering about the ignore_axes though, it seems simpler to document that the user should call copy!(dest.data, src.data) directly.

Any thoughts, @mbauman?

mdavezac commented 6 years ago

ping?

ajkeller34 commented 6 years ago

Not to leave you hanging, but at this point I think I've given what input I can, and I'll defer to others for merging. Unfortunately, everyone else is probably busy with getting Julia 0.7 ready...

iamed2 commented 6 years ago

I think we should remove the ignore_axes argument and instead suggest in the docstring that if the caller would like to ignore the axes they should use copy!(dest.data, src.data).

I would merge after that change!