-
Notifications
You must be signed in to change notification settings - Fork 28
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
Support rekeying a KeyedArray #63
Conversation
Nightly error seems unrelated? |
nightly failed on my PR due to #57 |
src/names.jl
Outdated
can also pass `dimname => newkey`, or even `oldname => newname => newkey` to both `rename` | ||
and `rekey` the specified dimension. | ||
""" | ||
rekey(x::AbstractArray, k2::Tuple) = KeyedArray(keyless(x), k2) |
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 wonder if this method ought to be restricted to types which have axis keys?
Mostly because it seems a little odd verbally, and redundant to provide a constructor. But, on the other hand, maybe it's useful not to have to care?
Precedent:
julia> rename(rand(3), (:x,))
ERROR: MethodError: no method matching rename(::Vector{Float64}, ::Tuple{Symbol})
Closest candidates are:
rename(::NamedDimsArray, ::Any) at /Users/me/.julia/dev/NamedDims/src/name_operations.jl:14
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.
Yeah, I mostly did this to avoid needing the awkward rekey(A::Union{NdaKa{L,T,N}, KaNda{L,T,N}}, k2::Tuple) where {L, T, N}
syntax. I assume there isn't a better solution? I suppose if this code is being used more than read then it doesn't really matter, but it's really hard for my brain to parse that Union
type.
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.
Could be just A::Union{KeyedArray, NdaKa}
I think?
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.
Unless you wanted to split things so as to preserve the order of wrappers, with a separate A:: NdaKa
method which unwraps the names & puts them back on afterwards.
Failure in 1.6 is
Seems like that should be unrelated... I should take a look at some point. |
@mcabbott this would be nice to have, any chance you will have time to review at some point? |
src/names.jl
Outdated
can also pass `dimname => newkey`, or even `oldname => newname => newkey` to both `rename` | ||
and `rekey` the specified dimension. |
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.
Is rekeying and renaming at some time a common operation?
Mightn't it be clearer to seperate it into two operations?
I would feel more comfortable with this PR if it only rekeyed.
And then we could do a follow up PR with the rekey + rename, that could be reveiwed can considered seperately
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.
That's a good point. Also, should something like rename
be defined for NamedDimsArrays first? I like the syntax for doing both at the same time, but I agree that could be a separate PR.
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 believe there is a rename
in NamedDims.jl
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.
Are we still okay with the pairs syntax or do we want to use kwargs?
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.
Okay, I've just dropped the rekey and rename method for now. I think that kind of functionality is only common in the DataFrames ecosystem (combine
).
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.
Approved subject to splitting the rename+rekey into a seperate PR
Will likely fail CI with the new |
I decided to go with pairs vs kwargs because:
oldname => newname => newvalues
seemed more intuitive than mixing pairs and kwargs.Closes #60