Fix failing XArray tests #97
Conversation
The issue was in calling `cat` on AxisArrays and KeyedArrays. For AxisArrays the issue was it wouldn't allow two arrays to have the same axis values. KeyedArrays allows cat-ing two arrays with the same values but the default axis keys generated in the tests did not match those from the code.
Codecov Report
@@ Coverage Diff @@
## main #97 +/- ##
==========================================
- Coverage 99.25% 98.52% -0.73%
==========================================
Files 12 12
Lines 134 136 +2
==========================================
+ Hits 133 134 +1
- Misses 1 2 +1
Continue to review full report at Codecov.
|
src/apply.jl
Outdated
| @@ -118,7 +118,8 @@ the usual [`Transform`](@ref) being invoked. | |||
| function apply_append(A::AbstractArray, t; append_dim, kwargs...)::AbstractArray | |||
| result = apply(A, t; kwargs...) | |||
| result = _postformat(cardinality(t), result, A, append_dim) | |||
| return cat(A, result; dims=append_dim) | |||
| # Call parent to avoid clashing axis/key names in concatenated result | |||
| return cat(A, parent(result); dims=append_dim) | |||
There was a problem hiding this comment.
Are we sure this is OK for if the arrays are some kind of SubArray or ReshapedArray or Symmetric etc?
There was a problem hiding this comment.
The first result should be a new instance of the array A - at least this is guaranteed for the FakeTransforms since those use replace to generate a result.
It can potentially get reshaped when calling _postformat but I added a call to copy to also return a new instance there.
I tried Symmetric, NameDimsArray on a few transforms and they worked so I think it's safe.
There was a problem hiding this comment.
added tests for NamedDimsArrays, SubArrays, and ReshapedArrays.
Symmetric is a bit odd because you can't call replace on it so it's not compatible with the test suite.
ERROR: ArgumentError: Cannot set a non-diagonal index in a symmetric matrix
Stacktrace:
Fixes #96
The issue was in calling
caton AxisArrays and KeyedArrays.For AxisArrays the issue was it wouldn't allow two arrays to have the same axis values.
KeyedArrays allows cat-ing two arrays with the same values but the default axis keys generated in the tests did not match those from the code.
Solution is to call
parenton either before callingcat, which removes the axis info and forces to use the default in both cases.