Submission Group 25: rbccovideda (R)

name: rbccovideda about: simple and scalable package for EDA of Covid19 cases in BC

Submitting Author Name: Lianna Hovhannisyan @liannah, John Lee @johnwslee, Vadim Taskaev @vtaskaev1, Vanessa Yuen @imtvwy Submitting Author Github Handle: @johnwslee Other Package Authors Github handles: @liannah, @vtaskaev1, @imtvwy Repository: https://github.com/UBC-MDS/rbccovideda Version submitted: Submission type: Standard Editor: Lianna Hovhannisyan @liannah, John Lee @johnwslee, Vadim Taskaev @vtaskaev1, Vanessa Yuen @imtvwy Reviewers: Simon Guo @y248guo, Kiran Phaterpekar @kphaterp, Rong Li @lirnish, Jessie Wong @jessie14

Language: en

Paste the full DESCRIPTION file inside a code block below:

Package: rbccovideda
Title: EDA Analysis for BC COVID19 data
Version: 0.0.0.9000
Authors@R: 
    person("Lianna", "Hovhannisyan", , "liannahovhannisyan11@gmail.com", role = c("aut", "cre"))
    person("John", "Lee", , "john.ws.lee@gmail.com", role = c("aut", "cre"))
    person("Vadim", "Taskaev", , "vadim.taskaev@outlook.com", role = c("aut", "cre"))
    person("Vanessa", "Yuen", , "imtvwy@gmail.com", role = c("aut", "cre"))
Description: Convenience functions to make the COVID19 Data analysis for British Columbia, CA region.
License: MIT + file LICENSE
Encoding: UTF-8
LazyData: true
Roxygen: list(markdown = TRUE)
RoxygenNote: 7.1.2
Imports: 
    downloader,
    dplyr,
    ggplot2,
    here,
    lubridate,
    RCurl,
    readr,
    scales,
    sjmisc,
    testthat,
    tibble,
    tidyverse
Config/testthat/edition: 3

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.):
- [x] data retrieval
- [ ] data extraction
- [x] data munging
- [ ] data deposition
- [ ] workflow automation
- [ ] version control
- [ ] citation management and bibliometrics
- [ ] scientific software wrappers
- [ ] field and lab reproducibility tools
- [ ] database software bindings
- [ ] geospatial data
- [ ] text analysis
Explain how and why the package falls under these categories (briefly, 1-2 sentences):

bccovideda package has two functions to generate line plot and histogram by using the data from BCCDC.
Who is the target audience and what are scientific applications of this package?

Analysts and decision-makers looking to track timely into regional BC COVID-19 cases Data science community members looking for basic package framework to build upon
Are there other R packages that accomplish the same thing? If so, how does yours differ or meet our criteria for best-in-category?

There are other packages, but those are specific to other country/regions.
(If applicable) Does your package comply with our guidance around Ethics, Data Privacy and Human Subjects Research?
If you made a pre-submission inquiry, please paste the link to the corresponding issue, forum post, or other discussion, or @tag the editor you contacted.
Explain reasons for any pkgcheck items which your package is unable to pass.

Technical checks

Confirm each of the following by checking the box.

[x] I have read the guide for authors and rOpenSci packaging guide.

This package:

[x] does not violate the Terms of Service of any service it interacts with.
[x] has a CRAN and OSI accepted license.
[x] contains a README with instructions for installing the development version.
[x] includes documentation with examples for all functions, created with roxygen2.
[x] contains a vignette with examples of its essential functions and uses.
[x] has a test suite.
[x] has continuous integration, including reporting of test coverage using services such as Travis CI, Coveralls and/or CodeCov.

Publication options

[ ] Do you intend for this package to go on CRAN?
[ ] Do you intend for this package to go on Bioconductor?
[ ] Do you wish to submit an Applications Article about your package to Methods in Ecology and Evolution? If so:

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

[x] I agree to abide by rOpenSci's Code of Conduct during the review process and in maintaining my package should it be accepted.

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

Briefly describe any working relationship you have (had) with the package authors.
[x] As the reviewer I confirm that there are no conflicts of interest for me to review this work (if you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

[x] A statement of need: clearly stating problems the software is designed to solve and its target audience in README
[x] Installation instructions: for the development version of package and any non-standard dependencies in README
[x] Vignette(s): demonstrating major functionality that runs successfully locally
[x] Function Documentation: for all exported functions
[x] Examples: (that run successfully locally) for all exported functions
[x] Community guidelines: including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).

Functionality

[x] Installation: Installation succeeds as documented.
[x] Functionality: Any functional claims of the software been confirmed.
[x] Performance: Any performance claims of the software been confirmed.
[x] Automated tests: Unit tests cover essential functions of the package and a reasonable range of inputs and conditions. All tests pass on the local machine.
[x] Packaging guidelines: The package conforms to the rOpenSci packaging guidelines.

Estimated hours spent reviewing: 1.5 hours

[x] Should the author(s) deem it appropriate, I agree to be acknowledged as a package reviewer ("rev" role) in the package DESCRIPTION file.

Review Comments

There is a slight inconsistency in your Usage instructions in the README file. When you are instructing the users on how to use your functions, you specify the actual values of the arguments in the markdown text instructions for plot_hist_by_cond. In contrast, you specify the names of the parameters instead in the markdown text instructions for the functions show_summary_stat and plot_line_by_date (i.e. plot_line_by_date(startDate, endDate)). Not a major issue, but an inconsistency that you may want to address.
Your scripts are very well written. One suggestion is that the get_data.R script doesn't have any comments for the code. If you are able to add some comments to this code it can help with its readability.
You have lots of tests for each function which is fantastic. Really makes your package functions robust and resistant to errors. Again, another small suggestion would be to add some comments to the test-get_data.R script just to make it easy to see what the tests are doing at a glance. Very minor issue though.
Friendly reminder to not forget to include a link to your package website (via pkgdown) before you submit! I see that the vignette has already been created which is great, so don't forget to include the link to the website that is accessible on the README page.
I think your package is quite intriguing and honestly something that I would use out of interest. Some features that I thought would be interesting for you to include in the future would be to allow for more customizability of the plots. What if I wanted to use the viridis colour palette, or simply choose basic colours for the plots? Maybe it's possible to allow the user to filter for a specific region they want to observe in the plot_line_by_date function? I'm sure your group has thought of some creative implementation ideas and understandably you can only do so much for a package within a given time frame. These are merely features that popped up in my head when playing around with your functions.
Overall, great package. I found a lot of utility in your functions, while also feeling appropriately supported by the useful error messages that you created. Great work all of you!

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

Briefly describe any working relationship you have (had) with the package authors.
[x] As the reviewer I confirm that there are no conflicts of interest for me to review this work (if you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

[x] A statement of need: clearly stating problems the software is designed to solve and its target audience in README
[x] Installation instructions: for the development version of package and any non-standard dependencies in README
[x] Vignette(s): demonstrating major functionality that runs successfully locally
[x] Function Documentation: for all exported functions
[x] Examples: (that run successfully locally) for all exported functions
[x] Community guidelines: including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).

Functionality

[x] Installation: Installation succeeds as documented.
[x] Functionality: Any functional claims of the software been confirmed.
[x] Performance: Any performance claims of the software been confirmed.
[x] Automated tests: Unit tests cover essential functions of the package and a reasonable range of inputs and conditions. All tests pass on the local machine.
[x] Packaging guidelines: The package conforms to the rOpenSci packaging guidelines.

Estimated hours spent reviewing: 1.5 hours

[x] Should the author(s) deem it appropriate, I agree to be acknowledged as a package reviewer ("rev" role) in the package DESCRIPTION file.

Review Comments

Similar to the Python package, the Number of Cases is plotted on the x-axis in plot_hist_by_cond but on the y-axis in plot_line_by_date, which may be confusing to users. The standalone graphs are perfectly understandable, but it would be ideal to have consistency between the two graphs.
The vignette is clear and informative, however, is there a reason why the dplyr package is loaded separately? It is listed as a dependency of the rbccovideda package, and should already be loaded with the package. If there is a particular reason why it needs to be loaded separately, it would be helpful to clearly state and explain this somewhere.
The usage examples in the README are very simple and easy to understand. It would be useful to also show that region = "all" is an optional argument in the plot_line_by_date function. The vignette and function documentation show this, but its not clear just from the README
The plot_hist_by_cond function is easy to use and interpret. Is there a particular reason why the condition argument only accepts Region and Age? Although there are less categories, it could still be informative to plot histograms for Sex or Classification.
The plot_line_by_date and show_summary_stat functions are well written and executed. A future improvement to the plot_line_by_date function could be to allow the option to plot the total cases in BC over time, instead of plotting separate lines for each region. show_summary_stat would be even more useful if it accepted an input argument that allowed the user to calculate the summary statistics of a specific subset of the data, such as only for a certain region or age group.
Overall, I think this package is well documented, has useful functions, and is unique within the R ecosystem. Great job!

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

Briefly describe any working relationship you have (had) with the package authors.
[x] As the reviewer I confirm that there are no conflicts of interest for me to review this work (if you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

[x] A statement of need: clearly stating problems the software is designed to solve and its target audience in README
[x] Installation instructions: for the development version of package and any non-standard dependencies in README
[x] Vignette(s): demonstrating major functionality that runs successfully locally
[x] Function Documentation: for all exported functions
[x] Examples: (that run successfully locally) for all exported functions
[x] Community guidelines: including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).

Functionality

[x] Installation: Installation succeeds as documented.
[x] Functionality: Any functional claims of the software been confirmed.
[x] Performance: Any performance claims of the software been confirmed.
[ ] Automated tests: Unit tests cover essential functions of the package and a reasonable range of inputs and conditions. All tests pass on the local machine.
[x] Packaging guidelines: The package conforms to the rOpenSci packaging guidelines.

Estimated hours spent reviewing: 2h

[x] Should the author(s) deem it appropriate, I agree to be acknowledged as a package reviewer ("rev" role) in the package DESCRIPTION file.

Review Comments

I like all the functions in this package and our group is actually written something similar. And this is something suggested by TA when our group is facing the same issue: when one function is working on downloading data, it is not suggested to use this function inside the other function to download data again. Instead, downloading data and saving it as a data frame, and then passing that data frame to do plotting is recommended. I think this might be because we don't want to download data multiple times, which takes space and time.
Noticed that the plot_line_by_date function is a bit hard to understand with many lines, I am wondering if it is possible to use direct labelling. In that case, it is clearer for a user to understand which line represents which region.
As the previous reviewer suggests, the loading of the dplyr package seems weird. I try to load this package only and not dplyr, and all the functions seem to work fine for me (not a full test though). What is the particular reason to load it separately? Maybe specify the reason in the readme, or remove the line?
For a test of plot_line_by_date(1, 31), I was expecting to get an error message of 'startDate' should be character. However, what I get is this: Error in charToDate(x) : character string is not in a standard unambiguous format

I think this is because this block in plot_line_by_date:

if (is.na(as.Date(startDate, "%Y-%m-%d")) || is.na(as.Date(endDate, "%Y-%m-%d"))){
     stop("Incorrect date format, should be YYYY-MM-DD")
     }

is before this:

 if (!is.character(startDate)) {
     stop("'startDate' should be character.")
     }

It tries to convert the date before checking if it is a character or not. So maybe the order should be reverted.

This also results in another concern: Although all of the expect_error lines pass, it might not be the expected error message. I would recommend using this format to check the error messages, so that the previous problem could be noticed.:
```
expect_error(plot_line_by_date(1, 31), "'startDate' should be character.")
```
A tiny point: from a user's perspective, I would recommend always including the function parameter names in the README Usage. In that case, users could get some hints from the parameter names; or if they want to switch the order of the parameters, they can easily figure out how to do it. Although they could also get this information from the documentation, they may be confused when the first time they read the README. Again, this is just a very tiny issue.
One additional thing, the copyright holder in the license should be individual names I think, not 'rbccovideda authors'.
Despite some trivial and nitpicky things and some updates needed for testing, I think this package is well done! The functions are pretty clear and useful.

Package Review

The authors and I are all MDS students of the same cohort
[x] As the reviewer I confirm that there are no conflicts of interest for me to review this work (if you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

[x] A statement of need: clearly stating problems the software is designed to solve and its target audience in README
[x] Installation instructions: for the development version of the package and any non-standard dependencies in README
[x] Vignette(s): demonstrating major functionality that runs successfully locally
[x] Function Documentation: for all exported functions
[x] Examples: (that run successfully locally) for all exported functions
[x] Community guidelines: including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).

Functionality

[x] Installation: Installation succeeds as documented.
[x] Functionality: Any functional claims of the software have been confirmed.
[x] Performance: Any performance claims of the software have been confirmed.
[ ] Automated tests: Unit tests cover essential functions of the package and a reasonable range of inputs and conditions. All tests pass on the local machine.
[x] Packaging guidelines: The package conforms to the rOpenSci packaging guidelines.

Estimated hours spent reviewing: 2h

[x] Should the author(s) deem it appropriate, I agree to be acknowledged as a package reviewer ("rev" role) in the package DESCRIPTION file.

Review Comments

This is a very interesting package on trending topics, which can definitely be useful at certain levels. Most of the issues and comments have been addressed by fellow reviewers, as I would try to not repeat what others have mentioned, my apologies if there is any overlapping. Here are some of my comments regarding this package:

Detailed and clear README with full guidelines is what I see when I first open the GitHub repo of the package. The documentation is great and they are all sufficiently informative and all the required sections are included with code and output examples. One tiny suggestion I would have is since this project works are based on the public covid data if I were you I may include detailed information regarding the data sources in the Credits section or anywhere else that can be appropriate.
The structure of the repository is well-constructed and since most of the stuff is automatically built with the dev tools no further comments on that. However, I wonder if it is best practice or if there is a better alternative for the /test/case_data_sample.csv to be stored elsewhere, or is it truly necessary for testing. Totally understandable if it is required for the testing and looks like it might be the case based on the current testing setup, but having extra data in a CSV file stored in the testing directory looks a bit odd to me.
Function wise most of the implementation is pretty well coded, and they are easy to read with great documentation. Below are some of my suggestions and comments regarding each function.
For show_summary_stat, constant declaration and validation tests are all fine and good, but the code for getting each summary value might be redundant. In order to follow the DRY principle, it might be better to have a separate helper function within the script and use that to extract each summary statistic value.
For plot_line_by_date, very detailed messages for the parameter checking and great organized code, but I am not sure why the mask variable is needed as it just gets reassigned with the plot_data variable later, but this is definitely not a major issue though. Another part that I am unsure about is whether the Reported_Date variable within the plot_data variable is sorted since to get the min and max date from it, one seems to be assuming it is and the other used the max function.
For plot_hist_by_cond, no problem with the checking and the checking, although I suspect that line 22 is a quick fix for some testing, it is a bit confusing to read and not sure if it is necessary. Implementation wise there might a bit redundant for plotting and using a helper function could be better, but it is debatable since you only have two conditions. However, for future integration, as more conditionals might be added, it can be a good practice to still provide another function handling the plotting and reuse the function.
For get_data, a very straightforward function and many functions from the here package were used. I would suggest that to integrate the function with better flexibility for the users, for example, instead of only allowing them to have an output folder, a clear full or partial path would be helpful and maybe even print out some current working directory information to help them decide what kind of path to store. Another thing I would probably add on to is by the end of the function before the return (well... it's probably for the best practice in R you don't have a return, but again, not a big deal) is to print out a message regarding the storage of the output file, as you are modifying the local directory outside the program.
Generally speaking from a user's perspective, I might want the line plot curvier and smoother to see the trend, but this might just be a personal preference :^) However, the flexibility of the plot (like different types of plots) might be something to improve on for future integration or alternative implementation.

Overall it is a very awesome package, and great work Group 25!

UBC-MDS / software-review-2022