UBC-MDS / software-review-2022

0 stars 0 forks source link

Submission Group 27: covizr (R) #53

Open lirnish opened 2 years ago

lirnish commented 2 years ago

name: covizr about: R package for easily visualizing COVID-19 statistics


Submitting Author Name: Rong Li Submitting Author Github Handle: !--author1-->@lirnish<!--end-author1-- Other Package Authors Github handles: @rrrohit1, @thomassiu, @ytz Repository: https://github.com/UBC-MDS/covizr Version submitted: 1.0.0 Submission type: Standard Editor: Rohit Rawat (@rrrohit1), Rong Li (@lirnish), Thomas Siu (@thomassiu), Ting Zhe Yan (@ytz) Reviewers: Sukhleen Kaur (@sukhleen999), Philson Chan (@PhilsChan), Kingslin Lv (@Kingslin0810)

Language: en


Scope

Confirm each of the following by checking the box.

This package:

Publication options

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

Kingslin0810 commented 2 years ago

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

Documentation

The package includes all the following forms of documentation:

Functionality

Estimated hours spent reviewing: 2 hours


Review Comments

Hello group 27, again congratulations and well done on this R package; similar to my comments for your python package, your repo is well organized and structured for anyone with basic R programming skills to visually read covid-19 data summary. The written documentation is clear for me to follow as well. I have a few notes for improvement, and some of these are the same comments to your python package:

  1. In your README I would suggest to include a section of Relevance to the R Ecosystem given there is a similar package like covid19
  2. Suggest you include a R-CMD-check state badge on README. Here is the tutorial and you are ready to add it now.
  3. Suggest to list get_data at first under Features
  4. Suggest to include a Documentation section in your README such as ?covizr::get_data for users to easily access.
  5. For plot_summary is sum the only aggerated function to input? I feel the Roxygen can be more explicitly to mention whether there are any other aggerated functions for the input.
  6. Again, in the Roxygen it would be helpful to clarify of how functions are interacting as of location. I am a little bit unclear and don't think to have a good solution other than updating the documentation. image
  7. When I run check() I got below NOTEs; I got similar note of "no visible binding for global variables" and was able to solve it by creating a file Gloabls.R. Detailed solution can be found here
    
    > checking top-level files ... NOTE
    Non-standard file/directory found at top level:
    'img'

checking R code for possible problems ... NOTE get_data: no visible binding for global variable 'iso_code' plot_summary: no visible binding for global variable 'value' Undefined global functions or variables: iso_code value

In addition to above comments, there are some missed check boxes above that you might still work on like Vigenttes. Again, great work everyone, please reach out if you have any questions. Thank you.

PhilsChan commented 2 years ago

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

Documentation

The package includes all the following forms of documentation:

Functionality

Estimated hours spent reviewing: 2 hours


Review Comments

The package overall looks neat and pretty. Similar to the python package, the charts are clear and visualize the data well. Unfortunately, I have encountered some technical problem occasionally so that I might not test it thoroughly. Below are some of my comments:

  1. When I first execute get_data, an error popped up as follow:

    > library(covizr)
    > df <- get_data(date_from="2022-01-01", date_to="2022-01-21")
    Error in value[[3L]](cond) : The link to the data is broken.                                                                                                              0s
    In addition: Warning messages:
    1: In base::nchar(wide_chars$test, type = "width") :
    restarting interrupted promise evaluation
    2: In base::nchar(wide_chars$test, type = "width") :
    internal error -3 in R_decompress1

    Although the error message suggested the link is broken, I could download the csv file directly from the link in your source code at the moment. However, the problem vanishes after a few hours. I think it would be valuable for you to keep a record of such problem.

  2. In the usage section of the README file, an equal sign was used for the assignment of output of get_data, which is not recommended by the tidyverse style.

  3. For the plotting functions, when there is no data in df within the date range specified by date_from and date_to, an empty plot would be shown. My personal suggestion would be throwing an error or showing customized warning in order to remind the users they might input a wrong range of dates.

  4. Similar to my 4th point in your python package, you could consider specifying the accepted values for the argument "fun", or make it accept function object as well. Or I think you might consider delete the argument if there is no other sensible options of aggregate functions.

  5. I have tried execute the following code, which is expected to give me the sum of new cases in 7 days by continent.

    > df <- get_data()
    > plot_summary(df, var="continent", val="new_cases", fun="sum", top_n=10)

    However, only 1 bar representing Oceania is visualized as shown below. error_chart I think there might be some logic error within the function.

  6. For plot_spec and plot_metric, I think it is a good idea to add the subtitle indicating the date range representated by the chart, just like how plot_summary did. I think it would make the package more unified, and increase the readablilty of the charts.

In general, you guys have done a great job and your package could certainly help a lot of people in lowering the barrier of data visualization. Wish you all the best!

sukhleen999 commented 2 years ago

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

Documentation

The package includes all the following forms of documentation:

Functionality

Estimated hours spent reviewing: 2 hours


Review Comments

The R package is equally good as the Python one and the instructions are easy to follow. The plots are concise yet informative and the package structure is is well defined which is reflective of the immense effort put in by the team!

Some pointers that might be considered for further improvement are:

  1. When I run the code below, I get the following error. I believe the function should return an error message specific to the incorrect date format as it is in the Python package. It would be even better if multiple date formats can be accepted.

    > plot_summary(df, var="location", val="new_cases", fun="sum", date_from="202-01-01", date_to="2022-01-15", top_n=10)
    Screen Shot 2022-02-05 at 5 14 26 PM
  2. The 'location' argument of the plot_spec function should handle erroneous input values by throwing an error.

    > df = get_data(date_from="2022-01-01", date_to="2022-01-21", location = c('Turkey', 'abdcde'))
    Screen Shot 2022-02-05 at 5 44 33 PM
  3. In the function plot_metric, the title of the plot does not adapt to the input for the 'loc_val' argument if multiple countries are passed.

    Screen Shot 2022-02-05 at 5 57 21 PM
  4. Finally, similar to my suggestion for the Python package, I recommend making the plots interactive for better insights into the data.

Overall, a great package and very well put together. Keep up the good work!