Allow setnafill() to accept logical vectors in cols (#4113) - #7842
Allow setnafill() to accept logical vectors in cols (#4113)#7842venom1204 wants to merge 6 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7842 +/- ##
==========================================
- Coverage 99.01% 99.01% -0.01%
==========================================
Files 88 88
Lines 17288 17293 +5
==========================================
+ Hits 17118 17122 +4
- Misses 170 171 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Generated via commit 986c878 Download link for the artifact containing the test results: ↓ atime-results.zip
|
| if (!is.list(x)) stop("in-place update is supported only for list", call. = FALSE) | ||
| cols = .Call(CcolnamesInt, x, cols, FALSE, FALSE) |
There was a problem hiding this comment.
Calling colnamesInt here seems the wrong approach to me, since we also call it in nafillR. Something simple as without additional C logic
if (is.logical(cols)) {
if (length(cols) != length(x)) stopf("'cols' is a logical vector of length %d but there are %d columns", length(cols), length(x))
if (anyNA(cols)) stopf("'cols' contains NA at position %d", which(is.na(cols))[1L])
cols = which(cols)
}| rm(inner, DT, DT2) | ||
|
|
||
| # #4113 setnafill could accept cols=logical(ncol(x)) | ||
| test(2382.01, {DT = data.table(a=c(1,NA,3), b=c(4,NA,6), c=c(7,NA,9)); DT1 = copy(DT); DT2 = copy(DT); setnafill(DT1, type="locf", cols=c(TRUE,FALSE,TRUE)); setnafill(DT2, type="locf", cols=c(1L,3L)); identical(DT1, DT2)}, TRUE) |
There was a problem hiding this comment.
please rather use a setup step, then trying to do everything in one step.
Also, most of the time we test against fixed values. If you really want to test integer indices versus logical indexing, I would suggest
| test(2382.01, {DT = data.table(a=c(1,NA,3), b=c(4,NA,6), c=c(7,NA,9)); DT1 = copy(DT); DT2 = copy(DT); setnafill(DT1, type="locf", cols=c(TRUE,FALSE,TRUE)); setnafill(DT2, type="locf", cols=c(1L,3L)); identical(DT1, DT2)}, TRUE) | |
| DT = data.table(a=c(1,NA,3), b=c(4,NA,6), c=c(7,NA,9)) | |
| test(2382.01, setnafill(copy(DT), type="locf", cols=c(TRUE,FALSE,TRUE)), setnafill(copy(DT), type="locf", cols=c(1L,3L))) |
|
|
||
| setnafill = function(x, type=c("const","locf","nocb"), fill=NA, nan=NA, cols=seq_along(x)) { | ||
| type = match.arg(type) | ||
| if (!is.list(x)) stop("in-place update is supported only for list", call. = FALSE) |

CLOSES #4113
this PR enables
setnafill()to accept logical vectors for the cols argument, allowing for dynamic column selection via conditions likesapply(DT, is.numeric). The implementation updates the internalcolnamesInt Cutility to handle logical vectors with exact length matching andNAvalidation while ensuring that existing error-handling logic remains consistent across the package.