436 fr function format sigfig j - #437
Conversation
There was a problem hiding this comment.
Overall, the idea looks good to me. Nevertheless, it would be good to clarify and potentially improve some of the design assumptions and implementation details, as indicated in my comments.
Please also check whether the CI/CD failure is related to the new code.
|
|
||
| #' @return numeric vector of the same length as `x`, rounded to `digits` significant figures. | ||
| #' @keywords internal | ||
| signif_j <- function(x, digits = 6, round_type = valid_round_type, whole_integer = FALSE, zero_threshold = 0) { |
There was a problem hiding this comment.
I'd aim for the most straightforward extension of base::signif(): preserve its behavior while allowing custom rounding, including the support for round_fun that does not support negative digits; and an optional zero_threshold.
I propose:
signif_j <- function(
x,
digits = 6,
zero_threshold = 0,
round_fun = round_fmt,
...
)Here, ... are passed as arguments to round_fun.
| if (whole_integer) { | ||
| # to ensure entire integer part is presented if x is greater than 10^digits | ||
| round_integer <- new_round < 0 | ||
| new_round[round_integer] <- 0 | ||
| } |
There was a problem hiding this comment.
Could we perhaps remove whole_integer here? What is the use case for it? As I mentioned above, I think it would be good to keep this function as consistent as possible with base::signif().
| x_f <- signif_j( | ||
| x, | ||
| digits = sigfig, | ||
| round_type = round_type, | ||
| whole_integer = whole_integer, | ||
| zero_threshold = zero_threshold | ||
| ) | ||
| num <- formatC(x_f, digits = sigfig, format = "fg", flag = flag) |
There was a problem hiding this comment.
This is easier to read and makes the PK reporting rule explicit. I also think it is safer because the threshold logic is handled directly here rather than being embedded in signif_j():
# Vectorized conditional handling
ifelse(
x < threshold,
formatC(signif_j(x, digits = sigfig, round_fun = round_fun, ...), digits = sigfig, flag = flag), # Up to threshold: round to sigfig significant digits
formatC(round_fun(x, digits = 0, ...)) # At or above threshold: report the whole integer
)(The details of the arguments can of course be adjusted.)
2 participants
Pull Request
Fixes #436
Checks