Change Adjoints to be ComponentArrays

nrontsis commented 1 year ago

This PR aims to avoid issues like:

using ComponentArrays
A = ComponentMatrix(ones(2, 2), Axis(:a, :b), FlatAxis())
A[:b, :] # works
A'[:, :b] # fails!

by wrapping adjoint operations in the underlying data of the ComponentArray structure.

By not having to care about adjoints of ComponentArray, we arguably also reduce overall cognitive load.

nrontsis commented 1 year ago

@jonniedie sorry for the broken tests, these are passing now in my machine.

YichengDWu commented 1 year ago

Why not just overload getindex for the adjoint?

nrontsis commented 1 year ago

Why not just overload getindex for the adjoint?

To reduce the size of this library and the number of concepts that it introduces
ComponentArrays either way allows for ComponentMatrix{Float64, Adjoint{Float64, Matrix{Float64}} that can be created like: ComponentMatrix(ones(2, 2)', Axis(:a, :b), FlatAxis()). There were thus two kind of adjoints: ComponentMatrix{Float64, Adjoint{Float64, Matrix{Float64}} and adjoint(::ComponentMatrix{Float64, Matrix{Float64}, resulting in confusion about which one to be used.
Besides the getindex issue, broadcasting seems to give incorrect results in the following example, as detailed in this comment:
```
julia> a = ComponentVector(ones(2), Axis(:a, :b))
julia> a .* a'
2×2 ComponentMatrix{Float64} with axes Axis(a = 1, b = 2) × Axis(a = 1, b = 2)
```
It's unclear what other edge cases we might be missing.

nrontsis commented 1 year ago

Thanks a lot for the review @jonniedie!

I am happy to add any tests you feel sensible. However I think some of the old tests were actually incorrect, as I argue in this thread. Can you reply to this thread with what you think on my argument?

jonniedie / ComponentArrays.jl

Change Adjoints to be ComponentArrays #170