mglu123
commented
4 years ago Hi! I will be able to review, but you might want to use the Varada's templates when you open this issue.
Open aromatic-toast opened 4 years ago
mglu123
commented
4 years ago Hi! I will be able to review, but you might want to use the Varada's templates when you open this issue.
aromatic-toast
commented
4 years ago She just changed this! This is the template she linked to in the Review Homework. Will fix this shortly.
mglu123
commented
4 years ago The package includes all the following forms of documentation:
URL, BugReports and Maintainer (which may be autogenerated via Authors@R).For packages co-submitting to JOSS
- [ ] The package has an obvious research application according to JOSS's definition
The package contains a
paper.mdmatching JOSS's requirements with:
- [ ] A short summary describing the high-level functionality of the software
- [ ] Authors: A list of authors with their affiliations
- [ ] A statement of need clearly stating problems the software is designed to solve and its target audience.
- [ ] References: with DOIs for all those that have one (e.g. papers, datasets, software).
fourier_transform is not workingfourier_transform is not workingfourier_transform is not working
and a reasonable range of inputs and conditions. All tests pass on the local machine.Estimated hours spent reviewing: 1 hr
Overall, it is a very practical package! You did a good job with writing tests, code coverage and generating the documentation site.
> devtools::check()
Updating ggexpress documentation
Loading ggexpress
Warning in (function (dep_name, dep_ver = "*") :
Dependency package 'vdiffr' not available.
Error: Dependency package(s) 'vdiffr' not available.
>library(ggexpress)
> my_data = tibble::tibble(time_series = c(0, 1, 2, 3), signal = c(2, 3, 4, 6))
> fourier_transform(data = my_data,
+ time_col = "time_series",
+ data_col = "signal")
Error in fourier_transform(data = my_data, time_col = "time_series", data_col = "signal") :
could not find function "fourier_transform"It seems that the package does not export functions properly.
[x] 3. It would be better to have example in README for function fourier_transform. Since other functions have code examples, it can be weird that fourier_transform does not have one.
[ ] 4. For the function fourier_transform, reference on this page (https://ubc-mds.github.io/ggexpress/reference/fourier_transform.html) gives a example with error. It would be better to give user a valid example.
[x] 5. For the plots generate from you functions, it would be better to have a better background, like theme_bw().
[ ] 6. Moreover, for scatter plot, it would be great if the user can change the opacity of the marker. Because if it is a huge data set, all markers might overlap together and will lead to the mistake of overfitting.
zanderhinton
commented
4 years ago The package includes all the following forms of documentation:
URL, BugReports and Maintainer (which may be autogenerated via Authors@R).For packages co-submitting to JOSS
- [ ] The package has an obvious research application according to JOSS's definition
The package contains a
paper.mdmatching JOSS's requirements with:
- [ ] A short summary describing the high-level functionality of the software
- [ ] Authors: A list of authors with their affiliations
- [ ] A statement of need clearly stating problems the software is designed to solve and its target audience.
- [ ] References: with DOIs for all those that have one (e.g. papers, datasets, software).
fourier_transformfourier_transformEstimated hours spent reviewing: 3 hr
General comments:
Areas of improvement:
[x] 1. I would remove vdiffr as a package requirement. I had issues installing it (as it appears the other reviewer also had), but upon inspection of your package it is not actually used (the only place where it is used is in the file test-ggscatter.R, but it is commented out). I locally removed it from DESCRIPTION and then devtools::check() ran without errors or warnings, nice!
[x] 2. The demonstration of fourier_transform is missing in both the README and the Vignette. Makes it more difficult to experiment with this function or see it's value in the overall scope of the package.
gghist and test-gghist:[ ] * Good use of defensive programming to make sure that the input is a dataframe, with a helpful error message. I would also add a personalized error message if the column provided is not a column in the dataframe.
[x] * This function only has one test case in the unit test. I do not have experience testing plot objects, but perhaps a few more could be thought of? For example, testing errors are thrown with bad inputs.
scatter_express and test-ggscatter:[ ] * Function could be made more defensive. It is good to check that columns are numeric, but there are a few other bad inputs I would want to test for. For example, throw a more specific error statement if the df is not a dataframe (see how this is handled in gghist). Additionally, I also think bad column input should be defended against.
[ ] * Perhaps could have more comments for read-ability
[ ] * Add error checking to test cases (check that function throws errors on bad input)
ts_plot and test-ts_plot[x] * Good defensive programming. Lots of input checking, great!
[x] * Thorough and easy to read test suite!
[ ] * If data which is not time series in nature is passed into this dataframe, this function still runs but will give some nonsensical results (example iris dataset and column Sepal.Length).
Given the input can be any data.frame I am not sure how to prevent this from happening, but perhaps something to think about.
fourier-transform and test-fourier[ ] * I would include more comments in this function to improve readability.
[ ] * Similar to above, I would verify the columns passed in are actually present in the df. This function does have quite good input checking though.
[ ] * In unit test, I would also check that errors messages are thrown with bad inputs.
7. Typos:
[x] * plotting taks in the README (line 91 of README.Rmd)
[x] * stastics in README, (line 95 of README.Rmd)
[x] * intial in Vignette, (line 20 of ggexpress-vignette.Rmd)
8. Style:
[x] * Use "<-" for assignment as much as possible instead of "=". This is noted in the test-fourier.R function
[ ] * Try to make lines shorter than 80 characters (this is recommended from goodpractice::gp()), all four functions have many lines longer than 80 characters. (This is quite nitpicky)
[x] * Avoid using library at the top of functions. It is preferred to use import in the documentation in packages. This occurs in the fourier_transform and ggscatter functions.
Overall I really enjoyed interacting with your functions, and I can see the benefits of quicker and easier EDA when using your package, well done!
zouwenjiao
commented
4 years ago Thanks for the insightful feedback @mglu123 @zanderhinton. We have worked through your suggestions and the following is what we have done.
From Luke's comments, we have implemented the following changes in our latest version:
Remove vdiffr as a package requirement to pass devtools::check().
Add an example in README for function fourier_transform.
Change plot background theme for gghist.
From Hinton's comments, we have done following improvements:
Remove vdiffr as a package requirement as he suggested.
Add the example of fourier_transform in both the README and the Vignette.
Fix typo in README
Add additional unit tests for gghist
Remove library imports in the fourier_transform.
Add comments to fourier_transform function.
Address style in the test-fourier.R function, using "<-" for assignment instead of "=".
The above improvements can be found in our latest version of release:
name: ggexpress
Submitting Author: Name Lesley Miller (@aromatic-toast) , Tejas Phaterpekar (@tejasph) , Jack Tan (@thejacktan), Wenjiao zou (@zouwenjiao). Repository: ggexpress Version submitted: 1.1.0 Editor: Verada Kolhatkar Reviewer 1: @zanderhinton Reviewer 2: @mglu123 Archive: TBD
Version accepted: TBD
Scope
Please indicate which category or categories from our package fit policies this package falls under: (Please check an appropriate box below. If you are unsure, we suggest you make a pre-submission inquiry.):
Explain how and why the package falls under these categories (briefly, 1-2 sentences): Our package automates the plotting process in ggplot2 when conducting Exploratory Data Analysis tasks. Instead of spending time to configure plots, a user can simply call our function and get a ready-made plot for generalized and niche plotting tasks.
Who is the target audience and what are scientific applications of this package?
Data scientists who need to rapidly visually explore their data.
Are there other R packages that accomplish the same thing? If so, how does yours differ or meet our criteria for best-in-category?
Please see the How this package fits into the R ecosytem section of our package README for details.
N/A
Technical checks
Confirm each of the following by checking the box.
This package:
Publication options
JOSS Options
- [ ] The package has an **obvious research application** according to [JOSS's definition](https://joss.readthedocs.io/en/latest/submitting.html#submission-requirements). - [ ] The package contains a `paper.md` matching [JOSS's requirements](https://joss.readthedocs.io/en/latest/submitting.html#what-should-my-paper-contain) with a high-level description in the package root or in `inst/`. - [ ] The package is deposited in a long-term repository with the DOI: - (*Do not submit your package separately to JOSS*)MEE Options
- [ ] The package is novel and will be of interest to the broad readership of the journal. - [ ] The manuscript describing the package is no longer than 3000 words. - [ ] You intend to archive the code for the package in a long-term repository which meets the requirements of the journal (see [MEE's Policy on Publishing Code](http://besjournals.onlinelibrary.wiley.com/hub/journal/10.1111/(ISSN)2041-210X/journal-resources/policy-on-publishing-code.html)) - (*Scope: Do consider MEE's [Aims and Scope](http://besjournals.onlinelibrary.wiley.com/hub/journal/10.1111/(ISSN)2041-210X/aims-and-scope/read-full-aims-and-scope.html) for your manuscript. We make no guarantee that your manuscript will be within MEE scope.*) - (*Although not required, we strongly recommend having a full manuscript prepared when you submit here.*) - (*Please do not submit your package separately to Methods in Ecology and Evolution*)Code of conduct