Collapse mm #61
Collapse mm #61
Conversation
# Conflicts: # DESCRIPTION
There was a problem hiding this comment.
Hi @zander-prinsloo ,
Thank you so much for the improvement. There are a few things that are still not clear to me. Let's talk about them before merging.
Thanks.
R/update_na_vals.R
Outdated
| dt_1[dt_1[[reportvar]] == 4, | ||
| x.var] <- dt_1[dt_1[[reportvar]] == 4, | ||
| y.var, | ||
| with = FALSE] | ||
| # | ||
| dt_1[dt_1[[reportvar]] == 5, | ||
| x.var] <- dt_1[dt_1[[reportvar]] == 5, | ||
| y.var, | ||
| with = FALSE] |
There was a problem hiding this comment.
Hi @zander-prinsloo , I don't understand this change. It is now making a deep copy rather than modifying by reference. What is the purpose of this change?
There was a problem hiding this comment.
@randrescastaneda , previously it was making a deep copy already using copy() and then modifying by reference after that. But you are right, this is not a sufficient improvement on what was done before. Let me propose a modification.
R/joyn-merge.R
Outdated
| # ensure input names can be restored | ||
| byexp <- grep(pattern = "==?", | ||
| x = by, | ||
| value = TRUE) | ||
| xbynames <- trimws(gsub("([^=]+)(\\s*==?\\s*)([^=]+)", | ||
| "\\1", | ||
| byexp)) | ||
| ybynames <- trimws(gsub("([^=]+)(\\s*==?\\s*)([^=]+)", | ||
| "\\3", | ||
| byexp)) |
There was a problem hiding this comment.
what happened with function fix_by_vars()?
R/joyn-merge.R
Outdated
| # in output | ||
| if (any(grepl(pattern = "keyby", x = names(x)))) { | ||
| data.table::setnames(x, | ||
| old = names(x)[grepl(pattern = "keyby", | ||
| x = names(x))], | ||
| new = xbynames) | ||
| } | ||
|
|
||
|
|
||
| # Change names back for inputs------------------------------ | ||
| if (any(grepl(pattern = "keyby", x = names(x_original)))) { | ||
|
|
||
| knames <- names(x_original)[grepl(pattern = "keyby", | ||
| x = names(x_original))] | ||
| knames <- knames[order(knames)] | ||
|
|
||
| data.table::setnames(x_original, | ||
| old = knames, | ||
| new = xbynames) | ||
| } | ||
|
|
||
| if (any(grepl(pattern = "keyby", x = names(y_original)))) { | ||
|
|
||
| knames <- names(y_original)[grepl(pattern = "keyby", | ||
| x = names(y_original))] | ||
| knames <- knames[order(knames)] | ||
|
|
||
| data.table::setnames(y_original, | ||
| old = knames, | ||
| new = ybynames) | ||
|
|
||
| if (all(names(y_original) %in% ynames)) { | ||
| colorderv(y_original, | ||
| neworder = ynames) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
When match_type is m:m this never hold because of the as.data.table() above, right? Is this by intention?
|
|
||
| ## Modify BY when is expression --------- | ||
| fixby <- check_by_vars(by, x, y) | ||
| by <- fixby$by | ||
|
|
||
| # Change names back on exit | ||
| # Change names back for inputs------------------------------ | ||
| on.exit( |
There was a problem hiding this comment.
I really like this idea. excellent.
R/dplyr-joins.R
Outdated
| @@ -520,6 +595,19 @@ inner_join <- function( | |||
| dropreport <- args_check$dropreport | |||
|
|
|||
| # Column names ----------------------------------- | |||
| byexp <- grep(pattern = "==?", | |||
There was a problem hiding this comment.
I think these lines of code should be converted to a function that can be called everywhere. Right now, the same code is repeated in multiple places.
# Conflicts: # NEWS.md # R/joyn-merge.R # R/joyn_workhorse.R # tests/testthat/test-joyn.R
|
@randrescastaneda Repetitive code made into a function. Merged with Note seen below was introduced by masking functions, specifically
|
Replace
merge.data.table()withcollapse::join()whenmatch_type = "m:m"All checks pass (excl. vignettes due to existing failures in DEV).