UBC-MDS / software-review-2022

0 stars 0 forks source link

Submission Group 02: Pylyrics2 (Python) #26

Open artanzand opened 2 years ago

artanzand commented 2 years ago

Submitting Author: Artan Zandian (@artanzand)
Package Name: pylyrics2 One-Line Description of Package: This package allows users to extract and analyze lyrics, and generate word clouds. Repository Link: https://github.com/UBC-MDS/pylyrics Version submitted: 1.0.7 Editors: Abhiket Gaurav @abhiket, Artan Zandian @artanzand, Macy Chan @MacyChan, Manju Abhinandana Kumar @manju-abhinandana Reviewer 1: Cici Du @ciciecho-ds Reviewer 2: Sanchit Singh @ssingh90 Reviewer 3: Shi Yan Wang @sy25wang

Description

Scope

* Please fill out a pre-submission inquiry before submitting a data visualization package. For more info, see notes on categories of our guidebook.

Technical checks

For details about the pyOpenSci packaging requirements, see our packaging guide. Confirm each of the following by checking the box. This package:

Publication options

JOSS Checks - [ ] The package has an **obvious research application** according to JOSS's definition in their [submission requirements][JossSubmissionRequirements]. Be aware that completing the pyOpenSci review process **does not** guarantee acceptance to JOSS. Be sure to read their submission requirements (linked above) if you are interested in submitting to JOSS. - [ ] The package is not a "minor utility" as defined by JOSS's [submission requirements][JossSubmissionRequirements]: "Minor ‘utility’ packages, including ‘thin’ API clients, are not acceptable." pyOpenSci welcomes these packages under "Data Retrieval", but JOSS has slightly different criteria. - [ ] The package contains a `paper.md` matching [JOSS's requirements][JossPaperRequirements] with a high-level description in the package root or in `inst/`. - [ ] The package is deposited in a long-term repository with the DOI: *Note: Do not submit your package separately to JOSS*

Are you OK with Reviewers Submitting Issues and/or pull requests to your Repo Directly?

This option will allow reviewers to open smaller issues that can then be linked to PR's rather than submitting a more dense text based review. It will also allow you to demonstrate addressing the issue via PR links.

Code of conduct

P.S. *Have feedback/comments about our review process? Leave a comment here

Editor and Review Templates

Editor and review templates can be found here

ciciecho-ds 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:

Readme requirements The package meets the readme requirements below:

The README should include, from top to bottom:

Usability

Reviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole. Package structure should follow general community best-practices. In general please consider:

Functionality

For packages co-submitting to JOSS

Note: Be sure to check this carefully, as JOSS's submission requirements and scope differ from pyOpenSci's in terms of what types of packages are accepted.

The package contains a paper.md matching JOSS's requirements with:

Final approval (post-review)

Estimated hours spent reviewing: 1


Review Comments

  1. I think the naming is a little confusing as pylyrics already exists and yours is not entirely iterating on top of it.
  2. You can add a description of the target user in the README. Something that speaks to why do I want to use this package (even just for fun!)
  3. README: I think you can combine the descriptions for the functions you have under the usage section with the features section so the usage section can be cleaner with just code (maybe a minimum amount of text to go along with it).
  4. I think you need to replace "it" with the function name for this part in your README: "clean_text() The lyrics extracted from extract_lyrics() are not clean. It removes special characters, html tags, #tags, contraction words and convert everything to lower case to get a cleaned paragraph."

Overall, great job!

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

Readme requirements The package meets the readme requirements below:

The README should include, from top to bottom:

Usability

Reviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole. Package structure should follow general community best-practices. In general please consider:

Functionality

For packages co-submitting to JOSS

Note: Be sure to check this carefully, as JOSS's submission requirements and scope differ from pyOpenSci's in terms of what types of packages are accepted.

The package contains a paper.md matching JOSS's requirements with:

Final approval (post-review)

Estimated hours spent reviewing: 1 hour


Review Comments

I am glad to have the opportunity reviewing this package as I enjoyed your presentation. Overall I think this is a well-structured package with a clear purpose. The package can be used or supplemented many other interesting ideas for sure.

I do have a few suggestions/recommendations:

  1. There's a problem installing the package which gives the following error message Building wheel for wordcloud (setup.py) ... error ERROR: Command errored out with exit status 1:...

    After some investigation, I think the issue is due to the wordcloud dependency. To solve the problem, users may use the following command cited from here

    py -3.9 -m pip install wordcloud-1.8.1-cp39-cp39-win_amd64.whl

    I would recommend including this information in your README file.

  2. I like the visualization for this package; I would recommend including the output image in your README for your plot function.
  3. There's a bit of inconsistency in your function docstrings. For example, download_data.py is missing the >>> for the example which is included for all other functions. I would recommend having consistency in wording and formats for all your functions.
  4. Under the Functions section of your README, the input column and the description together can lead to some confusing. For example, for clean_text function, the input mentions text, but the description refers to the lyrics.
  5. I agree with Cici's comment that sharing the same name with an existing package may not be a good idea. I would recommend changing the package name if possible (e.g., rename your repo as pylyrics2)

Overall, I see lots of efforts by the group and good job!

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

Readme requirements The package meets the readme requirements below:

The README should include, from top to bottom:

Usability

Reviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole. Package structure should follow general community best-practices. In general please consider:

Functionality

For packages co-submitting to JOSS

Note: Be sure to check this carefully, as JOSS's submission requirements and scope differ from pyOpenSci's in terms of what types of packages are accepted.

The package contains a paper.md matching JOSS's requirements with:

Final approval (post-review)

Estimated hours spent reviewing: 1.5


Review Comments

Great job on the package! Just a couple things I noticed and potential improvements for the future:

  1. There seems to be an issue with installation regarding building the wheel and the dependency on wordcloud. Shi Yan Wang's review addresses this.
  2. The documentation for function usage on readthedocs is done well and is easy to follow. It may be helpful to include a sample file and folder to demonstrate how to properly use kaggle for users who may be unfamiliar.
  3. It may be helpful in the future to include more edge cases for clean_text tests, such as a string with more irregular characters or spacing, as the current test case only tests for converting capital letters
  4. Adding a specific error output telling the user to fix their kaggle credentials in cases where there is difficulty authenticating kaggle credentials or finding the json file might be a useful addition to the download data function
  5. Since certain songs have multiple artists, it may be a good idea in the future to include documentation/functionality to support extracting lyrics for such songs. Genius also has some inconsistencies when it comes to artist names and capitalization, so it might be useful to include functionality to scan URLS for both capital and lowercase versions of artist names
Sanchit120496 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:

Readme requirements The package meets the readme requirements below:

The README should include, from top to bottom:

Usability

Reviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole. Package structure should follow general community best-practices. In general please consider:

Functionality

For packages co-submitting to JOSS

Note: Be sure to check this carefully, as JOSS's submission requirements and scope differ from pyOpenSci's in terms of what types of packages are accepted.

The package contains a paper.md matching JOSS's requirements with:

Final approval (post-review)

Estimated hours spent reviewing: 1 hr


Review Comments

Thoroughly enjoyed going through the package and it's functionality. Great job guys !

  1. I wasn't able to succesfully import the package, had to go through a solution on stack overflow to resolve it
  2. I believe some of these functions can be combined, such as downloading the data and extracting lyrics which will it easier for the users
  3. Since there is a makefile for the package, maybe mention it in the README with how to execute it
  4. The features section has less info about the functions when compared to the usage section. Just keeping the code in the usage should do or maybe shift most of the explanation to the feature section part
  5. Along with the word cloud, we can compare the word clouds of artist in the same genre to scale it for the user who would like to it not just for one particular singer