Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
Add HoD transform #11
Add HoD transform #11
Changes from all commits
8d74c29
3d96dc7
caf7285
ed0dc69
5fcc2d0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that's inconvenient. There is the syntax of
A[dimname1=inds, dimname2=inds, ...]
but I'm not sure if that can be written for alldimnames(A)
generically. There's also syntax ofA(dimname1=[axiskey11, axiskey12], dimname2=[axiskey21, axiskey22], ...)
which we could support.I don't mind this as-is for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused why this works for
KeyedArray
givenThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mapslices
x
is not a KeyedArray, it's a view of the arrayThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mapslices
here too?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works just as welll since it's mutating the array, I briefly tried using mapslices and things were breaking so I'd opt to leave as is. It does return the result we expect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't figure out why this returns a plain array?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we haven't asked
apply
to return a KeyedArray - just an array.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for
AxisArray
. If I followapply(A::AbstractArray, t::Transform; dims=:, inds=:, kwargs...)
then I think it does_apply
(without wrapping in an array), which I thought would preserve the type. But maybe this relates to your suggestion? i.e. it's currently callingapply!
when it should be_apply!
https://github.com/invenia/Transforms.jl/pull/11/files#r573720397There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah my mistake. Then this is
AxisArrays
being annoying and not conserving type when operating on them