allow Matrices in transformations of GroupedDataFrame#2782
Conversation
pdeffebach
left a comment
pdeffebach
left a comment
There was a problem hiding this comment.
It looks like all the action with select(df::AbstractDataFrame...) is happening inside manipulate here.
Why not use the same structure / logic with SplitApplyCombine?
|
We essentially use the same structure just the function is not called Originally I preferred to add type assertions in |
|
Okay. It sounds like we might want to re-factor in the future, but shouldn't use this PR to do it. |
|
Yes - refactoring is something I wanted to do for a long time, but you know how it is: we add features, the code gest more complex, at some point it is super hard to change anything unless you rewrite it from scratch (that is why I put so much attention to tests and documentation). |
| for cei in cs | ||
| @assert cei isa Union{Pair, Base.Callable, ColumnIndex, MultiColumnIndex} | ||
| @assert cei isa Union{Pair, Base.Callable, ColumnIndex, MultiColumnIndex, | ||
| AbstractVecOrMat{<:Pair}} |
There was a problem hiding this comment.
How were vectors supported before this PR?
There was a problem hiding this comment.
AbstractVector <: MultiColumnIndex
There was a problem hiding this comment.
I could add AbstractMatrix{<:Pair} in the union, but I thought that it is cleaner to use AbstractVecOrMat{<:Pair} to signal the design intention.
|
Thank you! |
3 participants
Fixes #2781