Fix *cat inconsistencies #169
Conversation
| idxmap_x, idxmap_y = indexmap.((ax_x, ax_y)) | ||
| return ComponentArray(vcat(data_x, data_y), Axis((;idxmap_x..., idxmap_y...))) | ||
| end | ||
| function Base._typed_hcat(::Type{T}, inputs::Base.AbstractVecOrTuple{ComponentArray}) where {T} |
There was a problem hiding this comment.
hcat and vcat call _typed_h/vcat under the hood.
Unfortunately, implementing the (seemingly private) _typed_hcat for ComponentArrays is apparently necessary for e.g.
reduce(hcat, [cv1, cv2])
to be identical to hcat(cv1, cv2), as the above calls directly _typed_hcat.
Since unit tests are in place for the above behaviour, I thought the above is okay-ish?
| axes_to_merge = [(getaxes(i)..., FlatAxis())[dims] for i in inputs] | ||
| rest_axes = [getaxes(i)[1:end .!= dims] for i in inputs] | ||
| no_duplicate_keys = (length(inputs) == 1 || isempty(intersect(keys.(axes_to_merge)...))) | ||
| if no_duplicate_keys && length(Set(rest_axes)) == 1 |
There was a problem hiding this comment.
Note the extra check length(Set(rest_axes)) == 1 that was not in place in the previous implementation.
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.
|
@jonniedie I fixed the tests here as well, after incorporating changes from #170. So perhaps first review #170. |
| @@ -148,7 +148,7 @@ Base.keys(ax::AbstractAxis) = keys(indexmap(ax)) | |||
| reindex(i, offset) = i .+ offset | |||
| reindex(ax::FlatAxis, _) = ax | |||
| reindex(ax::Axis, offset) = Axis(map(x->reindex(x, offset), indexmap(ax))) | |||
| reindex(ax::ViewAxis, offset) = ViewAxis(viewindex(ax) .+ offset, indexmap(ax)) | |||
| reindex(ax::ViewAxis{Inds,IdxMap,Ax}, offset) where {Inds, IdxMap, Ax} = ViewAxis(viewindex(ax) .+ offset, Ax()) | |||
There was a problem hiding this comment.
This is a bugfix that was only now discovered by the tests, because now cat calls reindex to all axis, as compared to only the first or the second one that the old hcats and vcats were calling before.
| @test vtempca isa ComponentVector | ||
| @test vtempca.r == temp.r | ||
| @test vtempca.c == ca.c | ||
| @test length(vtempca) == length(temp) + length(ca) | ||
| @test [ca; ca; ca] isa Vector | ||
| @test vcat(ca, 100) isa Vector | ||
| @test [ca' ca']' isa Vector | ||
| @test [ca' ca']' isa Adjoint{T, Matrix{T}} where T |
There was a problem hiding this comment.
This test was incorrect in my opinion, as in it was inconsistent to what happens with normal arrays.
Codecov Report
@@ Coverage Diff @@
## master #169 +/- ##
==========================================
+ Coverage 73.49% 75.79% +2.30%
==========================================
Files 20 20
Lines 679 599 -80
==========================================
- Hits 499 454 -45
+ Misses 180 145 -35
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
This PR addresses #113 and #168 by making cats support arbitrarily many arguments, and making
*cat(args...)equivalent toreduce(*cat, args...).Tests were extended accordingly.