-
-
Notifications
You must be signed in to change notification settings - Fork 104
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
Skimr #175
Comments
👋 @elinw |
@elinw thanks for your submission & sorry about the delay! I've seen development is still active, do you consider |
The tests now pass on my laptop using the latest He suggested to add |
Yes, as of now we have fixed the Windows issue in master. I'm about to push one more change and then I'm going to tag 1.0.1 sometime today. I'll definitely update the testthat minimum first. |
And you can confirm that 1.0.1 will be good for review? I even have two reviewers lined up once you confirm that. 😄 |
Yes it will be. |
Editor checks:
Editor commentsThanks again for your submission, if I understand correctly the version for review will be ready today?
It is good practice to
✖ write short and simple functions. These functions have high cyclomatic
complexity:kable.data.frame (60).
✖ avoid long code lines, it is bad for readability. Also, many people prefer editor
windows that are about 80 characters wide. Try make your lines shorter than 80
characters
R\skim.R:14:1
R\skim.R:73:1
R\stats.R:28:1
R\stats.R:52:1
R\stats.R:64:1
... and 20 more lines
Two typos identified via differnt skimr-package.Rd:16
faciliates skim_to_wide.Rd:19 Reviewers: @jimhester @jenniferthompson |
@jimhester and @jenniferthompson thanks for accepting to review this package! Your reviews are due on the 2018-01-30. 😺 You'll find here the review template and our reviewing guide. |
Thanks for the quick checks. I have done the easy ones, but there is one issue that we can't fix and two I'd like feedback on.
|
You can run covr on appveyor as well, and the coverage reports will be automatically merged by codecov. |
Cool, just add the same line to appveyor.yml? |
Appveyor needs to use double quotes and you can add it to on_success:
- Rscript -e "covr::codecov()" And I think you may need to add your codecov token to the appveyor settings. You can get the token at https://codecov.io/gh/ropenscilabs/skimr/settings, and add it as |
Thanks Jim! It seems to be working after ropensci/skimr@c34cec7 |
Release 1.0.1 should be in CRAN at this point. @michaelquinn32 is doing some refactoring that should be invisible to end users, and we have plans for updating some tests and documentation (and some feature requests) but other than that things are stable. |
@jimhester and @jenniferthompson friendly reminder that your reviews are due on Tuesday next week 😸 |
@michaelquinn32 @elinw FYI, I'm getting a "Project not found or access denied" error when I try to look at the Appveyor page via clicking the README badge. I'm not experienced in CI, so I'm not sure if that should have been addressed with the fix Jim mentioned a couple of weeks ago. |
Also, if I've caught some typos in the vignettes/examples, would y'all like a PR or no? |
Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe package includes all the following forms of documentation:
Functionality
Final approval (post-review)
Review CommentsAll review comments are based off of version 1.0.1. Overall I feel like the README
skimDisplaying the results of skim(iris, Sepal.Length, Petal.Length) %>%
filter(sd > 1) %>%
pull(variable) but this instead throws a pretty confusing error
Because the data is actually in long format. skim(iris, Sepal.Length, Petal.Length) %>%
filter(stat == "sd", value > 1) %>%
pull(variable) Gives you the expected output. As the print method hides this dichotomy I think this will cause users
Regardless of the exact API I think you should think hard about making it more I realize this is somewhat challenging because you have separate output for skim_withDesigning VignettesHaving the 'Using Fonts' vignette be a rmarkdown template is kind of strange, Miscellaneaget_funs() has a very weird implementation, particularly because it's behavior seems equivalent to double square bracket. e.g. The code inherited from I really think you should reconsider changing |
Thanks for your review @jimhester! 😸 |
skimr Package ReviewJennifer ThompsonThis review is of
DocumentationThe package includes all the following forms of documentation:
Functionality
Final approval (post-review)
Estimated hours spent reviewing: 5 Review Commentsskimr is a very, very useful package that is generally well thought in terms of both simplicity and extensibility. I particularly appreciate a thorough vignette for the main functions, intuitive function names, and the attention to where many users are likely to run into trouble, along with the proactive explanation of potential solutions. skimr does a specific set of tasks and does them simply and well. The source code is easy to digest in both style and function, and the comments are abundant, clear and helpful. I do agree with Jim that it's confusing to have the printed output be a completely different format from what is output to the console; I'm most excited about (and expect) the wide format printed, vs the long data.frame returned. Perhaps have the default Some specific suggestions:
Overall I'm excited to use this package in my own work, and really appreciate the hard and thoughtful work of all the authors. |
Thanks for the update @elinw and congrats on the release! 🎊 I guess you'll soon write the "final" answer to reviewers then? |
@maelle That's the plan. I wanted to add that we now have in the develop branch a solution to the other issue that I think folks here and in our tracker have wanted. That is now you can define your own complete list of functions with a name and use skim_with() to append or replace the default skimmers. So the way I think about this the use case would be that you have several different lists that you use for different situations e.g. my_funs1, my_funs2, my_funs3. Then you still have to do two steps but it is e.g.
I think if this were part of your usual workflow you might write your own functions so you don't need to do that There's also a PR making it so that if you define an empty list of functions for a type that type is removed from the analysis. |
Cool work! 😸 Thanks for the update. |
@elinw Any update? 😺 |
@elinw @michaelquinn32 any update? ☺ |
Hi,
Yes I think the development branch pretty much has addressed all, I just wanted to put in Jenny’s suggestion to make the display a bit nicer. I’ll do that once I recover from the unconfirmed! And then I think we could call it ready and everything else in terms of possibly changing the API etc will be for 2.0.
… On May 22, 2018, at 1:32 PM, Maëlle Salmon ***@***.***> wrote:
@elinw <https://github.com/elinw> @michaelquinn32 <https://github.com/michaelquinn32> any update? ☺
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#175 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAuEfd3bjl5B7xuc5tO8gmbV3rr7HvyJks5t1HX7gaJpZM4RIge_>.
|
Ok thanks. Don't hesitate to notify the reviewers in person 😉 |
Okay here's an update @jimhester @jenniferthompson @michaelquinn32 |
Changes for 2.0 are in this doc. I'm writing tests for the next version right now. |
Thanks for the updates @elinw and @michaelquinn32, very cool to see such active development! @jimhester and @jenniferthompson , are you happy with the current version of |
LGTM |
Very likely! I'm traveling and won't have time to look at the latest version till next week, but will do asap. |
Thanks @jimhester! @jenniferthompson, thank you, we'll wait for your input next week, happy travels! |
Sorry for the delay, y'all (jet lag is very real) - just looked at the updated version and it looks super! 🎉 Awesome work, @elinw @michaelquinn32. Going to check out the version 2.0 roadmap now 🏃♀️ |
Thanks @jenniferthompson @jimhester! |
I really appreciate all of the hard work everyone has put in. |
Approved! 🎊 Thanks a ton @jimhester and @jenniferthompson for your reviews and @elinw and @michaelquinn32 for your development work! Now here is the list of things you have to do before I close this issue 😉
Welcome aboard! We'd also love a blog post about your package, either a short-form intro to it (https://ropensci.org/tech-notes/) or long-form post with more narrative about its development. ((https://ropensci.org/blog/). If you are, @stefaniebutland will be in touch about content and timing. We've started putting together a gitbook with our best practice and tips, this chapter starts the 3d section that's about guidance for after onboarding. Please tell us what could be improved, the corresponding repo is here. |
@elinw @michaelquinn32 👋 could you soon tackle the list above? Thank you! 😃 |
Planned for this weekend!
Elin
… On Jun 30, 2018, at 7:53 AM, Maëlle Salmon ***@***.***> wrote:
@elinw <https://github.com/elinw> @michaelquinn32 <https://github.com/michaelquinn32> 👋 could you soon tackle the list above? Thank you! 😃
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#175 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAuEfYN1nk4oe9ge7tHOpAXahKbV1TYzks5uB2bLgaJpZM4RIge_>.
|
About Appveyor that currently doesn't have a badge because of some problem we had before but we do run it off of @michaelquinn32's account. So what to do? We're happy to start from scratch on it. |
You can keep the Appveyor project under @michaelquinn32's account if you prefer! |
I think it makes sense to put it in ropensci. |
Closing this, we can sort out CI details elsewhere and this way the peer-review badge will become green. 😉 |
@elinw Tired of writing about |
@jimhester @jenniferthompson @maelle I think we are finally close to release for v2 which addresses most of the issues we discussed, specifically the format of the skim object. We still need to rewrite vignettes and the readme. We'll be adding a pkgdown site although we're not making that as a blocker for the release. I think there is one important issue to resolve. (We'll probably do one more v1 release since we have fixed a few issues there in the past month.) @michaelquinn32 anything to add? |
Once you have the vignette I'd recommend making the pkgdown website right away because it'll be little effort and will allow you to put the link in DESCRIPTION before you submit the new version to CRAN 😉 |
Summary
What does this package do? (explain in 50 words or less):
Creates compact and flexible data summaries that are pipeable and display nicely in the console. For a given data frame or vector the skim function provides a useful set of summary statistics (based on the class of the individual vector/column) that allows users to skim their data to get an overall sense of what is included, extent of missing values, and similar information. The skim generic can be extended by users to other data structures.
Paste the full DESCRIPTION file inside a code block below:
URL for the package (the development repository, not a stylized html page):
https://github.com/ropenscilabs/skimr
Please indicate which category or categories from our package fit policies this package falls under *and why(? (e.g., data retrieval, reproducibility. If you are unsure, we suggest you make a pre-submission inquiry.):
"tools" Because skimr provides users with a way to get started with a new, unknown data set (by getting a quick overview (or skim) of the data that is readable and compact. It also serves as a good tool for reporting summary information about data.
yours differ or meet our criteria for best-in-category?
This is intended as an improved version of the r core summary functions available. Like summary, skim is a generic that users can extend to any R data object. It is designed to be somewhat like a more flexible version of fivenums() from
stats
or favstats() from the Mosaic package.Requirements
Confirm each of the following by checking the box. This package:
Publication options
paper.md
matching JOSS's requirements with a high-level description in the package root or ininst/
.Detail
Does
R CMD check
(ordevtools::check()
) succeed? Paste and describe any errors or warnings:Does the package conform to rOpenSci packaging guidelines? Please describe any exceptions:
If this is a resubmission following rejection, please explain the change in circumstances:
If possible, please provide recommendations of reviewers - those with experience with similar packages and/or likely users of your package - and their GitHub user names:
The text was updated successfully, but these errors were encountered: