Skip to content

Conversation

@kecsap42
Copy link

It's my solution

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@c4b35b1). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##             master     #2   +/-   ##
=======================================
  Coverage          ?   100%           
=======================================
  Files             ?      3           
  Lines             ?     18           
  Branches          ?      0           
=======================================
  Hits              ?     18           
  Misses            ?      0           
  Partials          ?      0
Impacted Files Coverage Δ
R/meanimpute.R 100% <ø> (ø)
R/transform_log.R 100% <100%> (ø)
R/windsorize.R 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c4b35b1...0034c46. Read the comment docs.

sibidev pushed a commit to sibidev/datacleaner that referenced this pull request May 4, 2019
* wrong implementation quantargo#1
* lacking wrong input handling quantargo#2
Copy link
Member

@mannau mannau left a 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")
Copy link
Member

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)) ){
Copy link
Member

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) ){
Copy link
Member

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.
Copy link
Member

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))){
Copy link
Member

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")
Copy link
Member

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants