Add limit parameter to nafill and setnafill #7677#7819
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7819 +/- ##
==========================================
- Coverage 99.02% 98.99% -0.03%
==========================================
Files 88 88
Lines 17266 17318 +52
==========================================
+ Hits 17097 17144 +47
- Misses 169 174 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Generated via commit 7c59afe Download link for the artifact containing the test results: ↓ atime-results.zip
|
| test(2379.01, nafill(c(1, NA, NA, NA, 5), type="locf", limit=1), c(1, 1, NA, NA, 5)) | ||
| test(2379.02, nafill(c(1, NA, NA, 5, NA, NA, NA, 9), type="locf", limit=2), c(1, 1, 1, 5, 5, 5, NA, 9)) | ||
| test(2379.03, nafill(c(1, NA, NA, 4, NA), type="locf", limit=Inf), c(1, 1, 1, 4, 4)) | ||
| test(2379.04, nafill(c(NA, NA, 3, NA, NA), type="locf", limit=1), c(NA, NA, 3, 3, NA)) |
There was a problem hiding this comment.
whats the difference between 01 and 04?
There was a problem hiding this comment.
can we also test edge cases like negative, 0, NA, NaN, NULL?
| # limit= restricts the number of consecutive fills | ||
| y = c(1, NA, NA, NA, 5) | ||
| nafill(y, "locf", limit=1) # Only fills the first NA | ||
| nafill(y, "locf", limit=2) # Fills the first two NAs |
There was a problem hiding this comment.
I dont get the added value of the second line, maybe change it to Inf for showcasing filling all.
| type = match.arg(type) | ||
| .Call(CnafillR, x, type, fill, nan_is_na(nan), FALSE, NULL) | ||
| if (!is.numeric(limit) || length(limit) != 1L || limit < 0) | ||
| stopf("limit must be a non-negative scalar numeric or Inf") |
There was a problem hiding this comment.
unprecise since Inf is also non-negative
There was a problem hiding this comment.
What about type="const". What about fractions like limit=1.5?
Please add tests for the following cases:
- nocb
- limit: 0, NA, NaN, negative, fractional, complex, NULL, and length > 1
- integer and character
- setnafill() by-reference
NEWS is missing.
What nafill(c(NA, 1), "locf", fill=0, limit=0)
67e1f87 to
dd875ac
Compare
| ans->dbl_v[0] = ISNAN(x[0]) ? fill : x[0]; | ||
| if (ISNAN(x[0])) fills = 1; |
There was a problem hiding this comment.
We do not need this double branching on ISNAN(x[0]). This will also happen at the other cases below!
| if (verbose) | ||
| tic = omp_get_wtime(); | ||
|
|
||
| double limit_val = REAL(limit)[0]; |
There was a problem hiding this comment.
Might be cleaner to represent as uint64_fast64_t since we also compare with it later
const double limit_d = REAL(limit)[0];
const uint_fast64_t limit_n = !R_FINITE(limit_d) || limit_d >= (double)UINT_FAST64_MAX ? UINT_FAST64_MAX : (uint_fast64_t)limit_d;
ben-schwen
left a comment
ben-schwen
left a comment
There was a problem hiding this comment.
Ty for the changes. We are almost there:
Still missing.
- the double branching thingy
- the c conversion of limit (this I consider as optional but it seems the right/cleaner approach)
- clarify in the docs whats happening for fractions of limit
- clarify whats happening for
type='const' - clarify boundary case for
nafill(c(NA, 1), "locf", fill=0, limit=0)which givesc(0, 1) - code coverage is still failing
2 participants
closes #7677
this PR adds a limit argument to nafill and setnafill to restrict the maximum number of consecutive NA values filled. Default is Inf to preserve existing behavior. Updated R interface, C kernel logic, and documentation.
hi @tdhock @joshhwuu can you please have a look at this when you got time thanks .