search

UBC-MDS / software-review

MDS Software Peer Review of MDS-created packages
1 stars 0 forks source link

Submission: rsketball #29

Open carlinakim opened 4 years ago

carlinakim commented 4 years ago

Submitting Author: Andres Pitta (@AndresPitta ), Kenneth Foo (@kfoofw ), Anand Shankar(@vanandsh ), Carlina Kim (@carlinakim ) Repository: rsketball Version submitted: 1.1.0 Editor: Varada Kolhatkar (@kvarada) Reviewer 1: Aakanksha Dimri (@aakanksha023)
Reviewer 2: Subing Cao (@scao1) Archive: TBD
Version accepted: TBD 

Package: rsketball
Title: Scrape data from ESPN NBA and show summary statistics/visualisations
Version: 0.0.0.9000
Authors@R: 
    person(given = "Kenneth",
           family = "Foo",
           role = c("aut", "cre"),
           email = "kfoofw@gmail.com");
    person(given = "Anand",
           family = "Vemparala",
           role = c("aut", "cre"),
           email = "vanandshankar@yahoo.com");
    person(given = "Carlina",
           family = "Kim",
           role = c("aut", "cre"),
           email = "carlinaa.kimm@gmail.com");
    person(given = "Andres",
           family = "Pitta",
           role = c("aut", "cre"),
           email = "apitta6@gmail.com")
Description: This package is designated for all NBA enthusiasts! The `rsketball` package works to scrape online tabular data from the ESPN NBA website into a csv file. It also includes various functions to create graphs and statistical analysis for your interest (such as boxplots, player rankings by stats, and a summary statistics table).
License: MIT + file LICENSE
Encoding: UTF-8
LazyData: true
RoxygenNote: 7.0.2
Suggests: 
    testthat (>= 2.1.0),
    covr,
    knitr,
    rmarkdown
Imports: 
    RSelenium,
    rvest,
    dplyr,
    forcats,
    ggplot2,
    stats,
    rlang,
    scales,
    magrittr,
    tibble,
    tidyr,
    xml2,
    utils,
    assertthat,
    checkmate,
    readr
URL: https://github.com/UBC-MDS/rsketball
BugReports: https://github.com/UBC-MDS/rsketball/issues
VignetteBuilder: knitr

Scope

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

scao1 commented 4 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:

For packages co-submitting to JOSS

The package contains a paper.md matching 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).

Functionality

Final approval (post-review)

Estimated hours spent reviewing:

Review Comments

This package is well-designed and is a very nice tool for its target audience. The instructions for installation and preparation is very detailed and easy to follow. The functions are well-written and user-friendly. Here are some small suggestions you may consider for future improvements.

  1. For the nba_scraper() documentation, the input for the ‘port’ argument should not only be positive integer but also be consistent with the the docker container settings. Although the authors mentioned that in the docker setup in the readme file, but it’s better also to indicate in the function documentation. Some grammar errors and typos were found in the description of of nba_scraper() documentation, such as the first sentence is not a complete sentence and the typo “seaason” should be changed to “season”. Please also check the nba_boxplot() documentation for grammar errors.

  2. The nba_boxplot() can produce a proper error message if there are misspellings in the input to the stats_column argument. But testing the the function with misspellings in the input to the grouping_list argument doesn’t raise an error message, instead the function just deletes the misspelled columns and generates the plot using the rest columns. In this case, it’s better to raise a warning message like “ Column names not found in the data were removed” to let the users know what happened. This is also the same with the nba_team_stats().

  3. It is nice that the authors make a reference to the website explaining each columns to help the users to understand the meaning of the columns. But instead of putting the reference at the end, the authors should consider to put it at the very beginning of “Usage examples” to help the users to select the right columns to analyze.

  4. The description for nba_boxplot() in the readme file is not very accurate. It is not to analyze "how different teams and positions affect different scoring statistics" but to compare the scoring statistics for different teams or positions.

  5. To make the titles of the plots more clear, I suggest changing the title of nba_boxplot() plot to something like "Comparing FT% scores in selected TEAMs" and the title of nba_ranking() plot to "Top 3 NAME based on 3PA ranking".

  6. Dependencies and the specific versions should be listed in Readme file (not only in the DESCRIPTION file).

Overall, this is a wonderful software. I really like it! Please let me know if any of the suggestions is not clear.

aakanksha023 commented 4 years ago

Package Review

Documentation

The package includes all the following forms of documentation:

For packages co-submitting to JOSS

The package contains a paper.md matching 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).

Functionality

Final approval (post-review)

Estimated hours spent reviewing: 2.5 hrs

Additional Comments:

Overall, the function documentation was crisp and easy to follow. Just faced a little difficulty with the function nba_boxplot. The wording could be simplified, If the author does not mind. One more minor suggestion common across all functions except scraping one (stelar work), was to reduce the number of examples to one to reduce the length of the documentation. The code for all 4 function were clean and quite easy to follow with appropriate comments.

Few glitches I noticed:

  1. nba_scraper()

    • Quite creative to display the msg "Data scraping of 2017 postseason season completed " at the end for non technical audience!
    • Impressive work with dynamic scrapping

  2. nba_boxplot()

    • An error msg in case of an unknown column in the argument grouping_list is missing. Rest all arguments produce appropriate error
    • The description of the function seems incorrect. The boxplot is helping the user compare stats for different teams/positions.

  3. nba_ranking()

    • Added functionality of rounding down the argument top automatically was a great touch instead of just displaying an error msg
    • With top=5, there seems to be some sizing issue. Unable to read the 5th name in both ascending and descending order
    • Would prefer a better colour scheme

  4. nba_team_stats()

    • An error msg in case of an invalid values for arguments positions_filter() and teams_filter() is missing. Argument stats_filter produces appropriate error msg with incorrect values

Overall, I thoroughly enjoyed the process. Look fwd to passing this along to few NBA enthusiasts post March 2020 release on CRAN (As promised in the Readme!)

kfoofw commented 4 years ago

Hi @scao1 and @aakanksha023, thanks for helping the review. We are performing some changes with respect to your feedback so feel free to track it in the following links:

Issue for feedback from @scao1: https://github.com/UBC-MDS/rsketball/issues/54

Issue for feedback from @aakanksha023 : https://github.com/UBC-MDS/rsketball/issues/55

Do note that not everything in your original review will be addressed but most of it will be. Once the pull request for the changes are collected, we will update this review thread.

Thanks once again!

kfoofw commented 4 years ago

Hi @scao1 and @aakanksha023, thank you for reviewing our package. We have addressed most of your suggested feedback so here is a summary of the actions taken:

Functions revision:

General Documentation revision:

Feedback issues not addressed:

For a more relevant look at the pull requests made, please refer to the following:

We are proud to release a new version 1.2.0 of rsketball. Please enjoy the revamped package!

Yours sincerely, @carlinakim @AndresPitta @vanandsh @kfoofw