Skip to content
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

Fixing all the Critical Points of datacleaner package for Quantargo training session. #4

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

nandakallugjeri
Copy link

@nandakallugjeri nandakallugjeri commented May 1, 2019

datacleaner package modified to fix all the critical points, mentioned in the last session of Quantargo's Data Science in Teams module, for the Preparation phase. Tests with testhat not included. #3

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!
Just spotted a few minor issues.

@@ -1,4 +1,8 @@
#' Meanimputation
#'
#' Removes NA-s with the mean value for the vector \code{x}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaces...

@@ -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 NAMESPACE file?

@@ -1,10 +1,20 @@
#' Windsorize
#'
#' Do some windsorization.
#' Replacing values of vector \code{x} greater or smaller then \code{q} quantile values.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: ... than ...

q <- quantile(x, p)
x[x >= q] <- q
if(length(x) == 0) stop('Empty vector passed as an argument.')
if( sum(is.na(x)) == length(x) ) stop('A vector of NA-s passed as an argument')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More explicit to use if (all(is.na(x))) ...

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.

2 participants