-
-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add tests and method for list in describe_distribution #105
Add tests and method for list in describe_distribution #105
Conversation
Codecov Report
@@ Coverage Diff @@
## master #105 +/- ##
==========================================
+ Coverage 73.12% 75.90% +2.78%
==========================================
Files 39 40 +1
Lines 2013 2042 +29
==========================================
+ Hits 1472 1550 +78
+ Misses 541 492 -49
Continue to review full report at Codecov.
|
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.
Looks good! Just a few minor comments that need to be attended to.
row.names(out) <- NULL | ||
out <- out[c("Variable", setdiff(colnames(out), "Variable"))] | ||
|
||
class(out) <- unique(c("parameters_distribution", "see_parameters_distribution", class(out))) |
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 tried the plotting method in {see}
and checked if it works with outputs from list
input?
Maybe you can post an example in the PR thread.
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.
I didn't know there was a plotting method for that. I just tried plot(describe_distribution(list(mtcars$mpg, mtcars$cyl)))
and it doesn't work. I don't have any experience with ggplot2
programming so not sure how to handle this. I'll take a look at see
source code later, maybe it's easier than I think
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.
I think you can leave this in for now and create an issue in {see}
repository so that we don't forget about it.
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.
Looks good to me! Thanks for working on this (and also for adding exhaustive unit tests!).
I will wait for @strengejacke to have a look at this before I merge.
This PR is related to #97. Sometimes we want to compare the distribution of several variables but not necessarily of the whole dataframe, so this PR adds a method to pass a list to
describe_distribution()
. Some examples below:Note that if the list is stored in an object (such as
x
above) and if its elements are unnamed, it is not possible to know the elements that createdx
, hence the "Var_1", "Var_2" as variable names. I also add tests and some doc for this.There will probably be some things to change so I didn't add details in the NEWS yet. What do you think?
Close #97.