-
Notifications
You must be signed in to change notification settings - Fork 14
Solution for the Preparation phase #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
…s below and above the specified percentile
…contains NA or is NULL
Fix issue quantargo#7: Handling NULL values corrected
Codecov Report
@@ Coverage Diff @@
## master #2 +/- ##
=======================================
Coverage ? 100%
=======================================
Files ? 3
Lines ? 18
Branches ? 0
=======================================
Hits ? 18
Misses ? 0
Partials ? 0
Continue to review full report at Codecov.
|
* wrong implementation quantargo#1 * lacking wrong input handling quantargo#2
mannau
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work!
Just a few minor issues regarding documentation and explicit use of any().
| @@ -1 +1,2 @@ | |||
| exportPattern("^[[:alpha:]]+") | |||
| importFrom("stats", "quantile") | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you used roxygen2 to generate the file here?
|
|
||
| transform_log <- function(x){ | ||
|
|
||
| if( sum(is.na(x)) ){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do NA values lead to an error here?
Sidenote: Although correct it would be more explicit to use any(is.na(x))
| else if( length(x) == 0 ){ | ||
| stop("Input x is NULL") | ||
| } | ||
| else if( sum(x <= 0) ){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, more explicit to use any(x <= 0). What could be the workaround for negative values here?
| @@ -1,10 +1,29 @@ | |||
| #' Windsorize | |||
| #' | |||
| #' Do some windsorization. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description should be more explicit.
| q <- quantile(x, p) | ||
| x[x >= q] <- q | ||
|
|
||
| if( sum(is.na(x))){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, any() is more explicit.
| x[x >= q] <- q | ||
|
|
||
| if( sum(is.na(x))){ | ||
| stop("Input x contains value NA") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to throw an error here? What could be the workaround for NA values?
It's my solution