Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix failing XArray tests #97

Merged
merged 5 commits into from
Jul 1, 2021
Merged

Fix failing XArray tests #97

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cfac90e
Fix failing XArray tests
Jun 30, 2021
3b27080
test on NamedDimsArrays
Jul 1, 2021
c7d687b
call Matrix instead of parent
Jul 1, 2021
bd8d4f9
bump patch version
Jul 1, 2021
cdb209a
test on Reshaped and Sub arrays
Jul 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/apply.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure this is OK for if the arrays are some kind of SubArray or ReshapedArray or Symmetric etc?

Copy link
Member Author

@glennmoy glennmoy Jun 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add them to tests?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

end

"""
Expand Down Expand Up @@ -153,5 +154,5 @@ _postformat(::Cardinality, result, A, append_dim) = result
function _postformat(::ManyToOne, result, A, append_dim)
new_size = collect(size(A))
setindex!(new_size, 1, dim(A, append_dim))
return reshape(result, new_size...)
return copy(reshape(result, new_size...)) # return a copy to remove the reshape type
end