- Notifications
You must be signed in to change notification settings - Fork374
Updateindex#3401
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Updateindex#3401
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…te columns when `makeunique` is false.If `mergeduplicates` is passed a function then that function is invoked on the values of all duplicate columns and its return value is assigned to that named column.
will only be done two at a time.
bkamins commentedNov 11, 2023
As I have commented before - it will be super hard to review such a big PR. That is why I have recommended to split it into smaller PRs and merge them incrementally. But I will try to comment on this PR (however, take note that because it is so big it will be hard to properly review it and be sure that all issues are caught/discussed). As a side note - it seems that you did not use latest |
| rename!(index(df), vals, makeunique=makeunique) | ||
| makeunique::Bool=false, mergeduplicates::MergeDuplicates=nothing) | ||
| if!makeunique&&isa(mergeduplicates, Function) | ||
| (new_columns, colindex)=process_updates(UpdateIndex(vals),_columns(df), mergeduplicates) |
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.
_columns is not defined for generalAbstractDataFrame.
| makeunique::Bool=false) | ||
| rename!(index(df), vals, makeunique=makeunique) | ||
| makeunique::Bool=false, mergeduplicates::MergeDuplicates=nothing) | ||
| if!makeunique&&isa(mergeduplicates, Function) |
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.
the docstring seems not to have been updated.
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.
in particular, I am not clear whatrename!/rename should do whenmergeduplicates is passed.
| return df | ||
| end | ||
| functionrename!(idx::Index, new_index::Index) |
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.
functions forIndex should be added in other/index.jl
| end | ||
| functionrename!(idx::Index, new_index::Index) | ||
| splice!(idx.names,1:length(idx.names), new_index.names) |
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.
should we first check thatidx andnew_index are independent?
| 1 │ 1 2 3 | ||
| ``` | ||
| """ | ||
| rename(df::AbstractDataFrame, vals::AbstractVector{Symbol}; |
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.
docstring update is missing
| Wherever the `mergeduplicates` keyword argument is available it is either `nothing` or | ||
| a `Function` that will be executed to combine duplicated columns (when `makeunique=false`) | ||
| """ | ||
| MergeDuplicates= Union{Nothing,Function} |
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 am OK to add this definition, but then its docstring should be more precise I think.
| will be combined by invoking the function with all values from those columns. | ||
| e.g. `mergeduplicates=coalesce` will use the first non-missing value. Since `hcat` and | ||
| `hcat!` are performed recursively for more than two frames, this `mergeduplicates` | ||
| function will only combine two columns at a time. |
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.
it is not clear what happens ifmakeuniqe=true andmergeduplicates isFunction`.
| Horizontally concatenate data frames. | ||
| If `makeunique=false` (the default) column names of passed objects must be unique. |
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 statement does not seem to be true after this PR.
| if it is `true` a new unique name will be generated by adding a suffix, | ||
| if it is `false` an error will be thrown unless a `mergeduplicates` functiom is provided. | ||
| - `mergeduplicates` : defines what to do if `name` already exists in `df` and `makeunique` | ||
| is false. It should be given a Function that combines the values of all of the duplicated |
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.
| isfalse. It should be given a Function that combines the values of all of the duplicated | |
| isfalse. It should be given a`Function` that combines the values of all of the duplicated |
| if it is `false` an error will be thrown unless a `mergeduplicates` functiom is provided. | ||
| - `mergeduplicates` : defines what to do if `name` already exists in `df` and `makeunique` | ||
| is false. It should be given a Function that combines the values of all of the duplicated | ||
| columns which will be passed as a varargs. The return value is used. |
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.
it is not clear if the passed function takes elements of the columns iteratively or whole columns.
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.
Also it is not clear how things are processed if multiple duplicate columns are provided.
bkamins commentedNov 11, 2023
I have not finished reviewing the PR. I will try to get back to it later. |
elainethale commentedOct 30, 2024
Estimate on when this functionality will be merged into DataFrames.jl? Has this PR been replaced with yet another one? |
Uh oh!
There was an error while loading.Please reload this page.
The is a replacement for#3366. It defines a new keyword
mergeduplicatesthat can be set to aFunctionthat combines values to merge columns instead of erroring when there are duplicate column names andmakeunique=false.It is implemented in stages, the first of which creates a temporary struct
UpdateIndexwhich is used to initialize aDataFrameor represent a set of column names and columns that will be merged into the resultingDataFramein anhcatorhcat!operation. It then follows up with commits that extendpermutedimsand joins to resolve column clashes in the same fashion.