Quantargo exercise - modify existing package #13
Conversation
Fix usage example of windorize()
Codecov Report
@@ Coverage Diff @@
## master #13 +/- ##
=========================================
Coverage ? 90.47%
=========================================
Files ? 3
Lines ? 21
Branches ? 0
=========================================
Hits ? 19
Misses ? 2
Partials ? 0
Continue to review full report at Codecov.
|
mannau
left a comment
mannau
left a comment
There was a problem hiding this comment.
Great work!
Just found a few minor issues in the code.
| @@ -1 +1,2 @@ | |||
| exportPattern("^[[:alpha:]]+") | |||
| importFrom("stats", "quantile") | |||
There was a problem hiding this comment.
It does not seem that the file has been created with roxygen2. Why?
| @@ -1,4 +1,5 @@ | |||
| #' Meanimputation | |||
| #' @param x A vector | |||
There was a problem hiding this comment.
Please include a description of the function here.
| @@ -0,0 +1,19 @@ | |||
| #' transform_log | |||
| #' Transform numerical values into their log values | |||
There was a problem hiding this comment.
You should include an empty roxygen line between header and description.
| @@ -0,0 +1,19 @@ | |||
| #' transform_log | |||
| #' Transform numerical values into their log values | |||
| #' @param x A vector | |||
There was a problem hiding this comment.
Parameter description could be more specific here, e.g. indicating the data type.
| #' | ||
| transform_log <- function(x) { | ||
| if (!is.numeric(x)) { | ||
| warning("transform_log: Expecting numeric argument") |
There was a problem hiding this comment.
You should stop here instead of indicating a warning. Implicit data type conversions can always lead to strange results.
| if (!is.numeric(x)) { | ||
| warning("transform_log: Expecting numeric argument") | ||
| } | ||
| x_badval <- is.na(suppressWarnings(as.numeric(x))) |
There was a problem hiding this comment.
No implicit conversion should be done here, see previous comment.
| if (p < 0 || p > 1) { | ||
| stop("p invalid percentale. Expected values from 0 to 1") | ||
| } | ||
| q_lower <- quantile(x, (1-p)/2) |
There was a problem hiding this comment.
What happens if vector x contains some NA values here?
3 participants
All issues are fixed.