UBC-MDS / software-review

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

Submission: Rpuck #32

Open PolinaRomanchenko opened 4 years ago

PolinaRomanchenko commented 4 years ago

Submitting Author: Xugang Zhong (@chuusan ), Jarvis Nederlof (@jnederlo), Polina Romanchenko (@PolinaRomanchenko), Manish Joshi (@ManishPJoshi),
Repository: rpuck Version submitted: v1.1.0 Editor: Varada Kolhatkar (@kvarada) Reviewer 1: Yue Jiang (@YueJiangMDSV) Reviewer 2: Ruidan Ni (@rita-ni) Archive: TBD
Version accepted: TBD


Package: rpuck
Title: What the Package Does (One Line, Title Case)
Version: 0.0.0.9000
Authors@R: 
    person(given = "Kirk",
           family = "Zhong",
           role = c("aut", "cre"),
           email = "chuusankirk@hotmail.com",
           comment = c(ORCID = "YOUR-ORCID-ID"))
Description: This package provides wrapper functions to access the publicly available but undocumented NHL.com API.
License: MIT + file LICENSE
Encoding: UTF-8
LazyData: true
RoxygenNote: 7.0.2
Imports:
    dplyr,
    httr,
    ggplot2,
    jsonlite,
    grid,
    gridExtra,
    glue,
    tidyr,
    testthat,
    scales
Suggests: 
    covr,
    knitr,
    rmarkdown
URL: https://github.com/UBC-MDS/rpuck
BugReports: https://github.com/UBC-MDS/rpuck/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

rita-ni 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

As a person not very familiar with hockey sports, I found your project repo to be very informative and easy to understand. Your README was very well structured, after reading your README I can understand well what your package is intended for, and the usage examples are very helpful to show what each function in your package is trying to achieve. The dependencies are also clearly listed, which is easy for users to check on prior to installing the package. Great work!

Also, the package logo is a nice touch to the project repo!

Some possible room for improvements:

Screen Shot 2020-03-21 at 3 59 19 PM

I played around with your package a bit so perhaps this feedback is a bit long, it is a fun project! I am happy to have the opportunity to try out and review your package. Please feel free to let me know if you have any questions regarding my feedback.

YueJiangMDSV 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:

3 hours


Review Comments

Hi everyone, below are my comments:

Well done everyone! The package is really one of a kind. It focuses on a niche market and a special group of target users. I do not think there are any existing package that has the same functionality. The package makes query NHL data fast and easy. For people have no idea how to make API calls or how to scrape data from websites, but are interested in analyzing on NHL data, this package would be of great help.

I personally have no experience in the NHL data, but the package is really easy to use.

Here are some highlights:

  1. The installation process through devtools is smooth, with no issues or no warnings.
  2. I ran check() in R, and it passed all the tests, no warnings, no errors. Really good job!
  3. The package is well-designed and well-documented. The doc strings are clear and succinct, and the examples are easy to understand. It does not take me much longer to understand what are the functions, what can they do, and how to use them.
  4. In the README, all the dependencies are listed, especially their version number, which is great! Now the users know what packages and which versions they need to use this package.

Here are some suggestions:

  1. The REAME includes installation from CRAN, which does not work…. maybe consider deleting that part?
  2. There is no link to the pkgdown website, and there is no link to the vignette. However, I do manage to find the pkgdown website and the vignette (I cloned your repo…and find the html file for pkgdown website). The vignette is exactly the same as the README. I don’t think the vignette and the README are the same thing…otherwise, there is no need to have the vignette. The vignette could be more user-friendly, and in a softer tone to go over all the features/functions in the package. There is no need for the vignette to include installation instructions or dependency list I believe.
  3. The package currently is on version v1.1.0, but the documentation says version 0.0.0.9. Maybe consider updating the DESCRIPTION file?
  4. The error messages could be more informative. For example, I entered draft_pick(pick_number = 0) and draft_pick(pick_number = "w"), the error message is the same, saying “Pick number out of range”. It might be better to say why pick_number cannot be 0, or why it cannot be a string. Same thing applies to round_number and year.
  5. Is there a way to improve the aesthetics of the plots for the attendance function? When I tried attendance(regular=TRUE, playoffs=TRUE, start_season= 2000, end_season=2018), or even longer time range, the labels for the x-axis is hard to read. In might be a good idea to put the two plots vertically, instead of being side-by-side. Additionally, when the time range is short, say 2014 to 2018, the attendance for regular season is almost flat. It is hard to see the changes with attendance is more than 20 million each.
  6. If I just type player_stats(start_date = "2019-10-02", end_date = "2020-02-28") in the R console, it does not return anything. I have to assign the function to a variable, and then call that variable to view the results. I looked at the code of the function, and find that the function indeed does not return anything. Same as team_stats() function. It does not return anything if I just type team_stats() in the R console.
  7. It may be a good idea to do some pre-wrangling for the returned player_stats() data frame and team_stats() data frame? It is hard for me to find the players’ names or the teams’ names in the current output. The result has many columns that cannot be displayed in one line, and it runs multiple lines. The columns are arranged in alphabetical order, and the players’ names are listed in the last but one column. Thus, for someone not that familiar with the data frame, it may be a little hard to find the players’ names. It may also be a good idea to add the players’ names in the query, so the user can pass the players’ names as an input in the function, and get the results directly, instead of do the wrangling on their own.
  8. There seems to be a small typo in test-attendance.R, line 27 to 30:
    test_that("Error message should be expected if regular is set to non_logical value", {
    expect_error(attendance(regular = TRUE, playoffs = 1, start_season = 1996, end_season = 2000),
                 regexp = "Regular and playoffs must be logical values!")
    })

Here, clearly, regular is set to logical value, and playoffs is set to non-logical value. But the test_that message is “Error message should be expected if regular is set to non_logical value”

  1. In the README, under Usage Example, for the team_stats() function,
attendance <- team_stats(start_season = "19801981", end_season = "19891990")
head(attendance)

Although it is free of choice to assign the value to any variable, assigning the result to a variable called attendance seems confusing, especially when there is a function called attendance()

Above are just minor suggestions. Overall, I think this is a great package and can be really useful for people analyzing NHL data. I really enjoyed reviewing your package and I am really impressed by the functionalities of the package. Please let me know if you have any questions on my feedbacks. I am happy to discuss.

chuusan commented 4 years ago

@YueJiangMDSV First of all, thank you for the suggestions. In terms of the link to the pkgdown website, we put it on the top of repo as Tiffany's demo. Regarding to the vignette link, we did successfully build it and the link was accidentally missing from our README. Gonna update the README in our final work.

Screen Shot 2020-03-22 at 12 31 33 PM

Thank for the suggestions!

YueJiangMDSV commented 4 years ago

@YueJiangMDSV First of all, thank you for the suggestions. In terms of the link to the pkgdown website, we put it on the top of repo as Tiffany's demo. Regarding to the vignette link, we did successfully build it and the link was accidentally missing from our README. Gonna update the README in our final work.

Screen Shot 2020-03-22 at 12 31 33 PM

Thank for the suggestions!

Oops! Missed that...Sorry about that! Thanks for pointing out!

PolinaRomanchenko commented 4 years ago

Hi @YueJiangMDSV and @rita-ni! Thank you all for spending time and looking into our package so precisely! We went through and addressed all of the suggestions that didn't introduce new features as those can/should be addressed in future major releases. To specifically summarize what we addressed:

Regarding other suggestion's not addressed in this release team believes that those can/should be addressed in future major releases or were intentional choices we made for our project. You can view these changes as part of release v1.2.0.