UBC-MDS / software-review

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

Submission: hueniversitypy (Python) #9

Open reikookamoto opened 4 years ago

reikookamoto commented 4 years ago

name: Submit Software for Review about: Use to submit your Python package for peer review title: '' labels: 1/editor-checks, New Submission! assignees: ''


Submitting Author: Reiko Okamoto (@reikookamoto), Evelyn Moorhouse (@evelynmoorhouse), Simardeep Kaur (@SimardeepKaur), Shivam Verma (@vermashivam679) (Group 16) Package Name: hueniversitypy One-Line Description of Package: Python package for creating visualizations in line with visual identities of Canadian universities Repository Link: hueniversitypy Version submitted: v1.1.0 Editor: @kvarada Reviewer 1: @aromatic-toast Archive: TBD
Version accepted: TBD


Description

This Python package allows users to apply university-specific themes to Altair plots. This package currently supports the official colour palettes of four institutions belonging to the U15 Group of Canadian Research Universities: University of Alberta, the University of British Columbia, McGill University, and the University of Toronto. In the future, we hope to extend this package to support the visual identities of all universities in the association.

Scope

* Please fill out a pre-submission inquiry before submitting a data visualization package. For more info, see this section 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](https://joss.readthedocs.io/en/latest/submitting.html#submission-requirements). 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](https://joss.readthedocs.io/en/latest/submitting.html#submission-requirements): "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](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: *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

ttimbers commented 4 years ago

Package Review

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:

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

This is a really neat package that I definitely would use for formatting visualizations for presentations about my UBC teaching! What a great idea!

aromatic-toast commented 4 years ago

Package Review

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:

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


Review Comments

Overall Comments

This package is very cool and super useful. It was a joy to review it. All documentation, vignette and installation instructions were clear and easy to follow. Everything worked as expected and I was able run all examples in the README with no issues. I even ran the colour themes on different datasets and plots than what was provided in the README and everything worked as expected. Fantastic job!

Additional Thoughts/suggestions:

  1. It may be useful to a user to add test data and test plots in the example for every docstring. Then, a user may run an entire example from top to bottom and see the output instantly without visiting the package README.
  2. Under the Documentation section of the reviewer template it asks for Metadata. I wasn't able to check this off because I wasn't able to find this information if it exists.
  3. As I mentioned, I tested the package out on different datasets in addition to those in the README. I noticed that if a user makes a simple barchart of a single variable the color theme will not change. This seems reasonable since aesthetically there is no need to color every single bar a different color. The README barchart changes colors because it is plotting 2 different years. I'm not sure if a note about this behaviour should be included in the docstring just to alert users to this behavior if just in case this isn't what they are expecting.
  4. Since each theme is only 5-6 colors, plots that have more than this number of categories can not be uniquely identified by color. This isn't unique to your package as I've discovered this using other palette tools in the past. But perhaps this should be explicitly noted to the user? This is probably too nitpicky and I'm not even sure this is necessary since I've seen this issue before with other palettes in ggplot and no mention was made about this.
  5. Currently it seems the build is failing on both versions of Windows but this didn't effect me.
  6. There is a tiny typo in the first line of the first paragraph in the README under hueniversitypy in the Python ecosystem. (the word alongside is misspelled)
vermashivam679 commented 4 years ago

Thank you @aromatic-toast, for providing insightful feedback to our package. We tried to incorporate all the feedback given by you, and following is our update on that:

  1. It may be useful to a user to add test data and test plots in the example for every docstring. Then, a user may run an entire example from top to bottom and see the output instantly without visiting the package README.
    • Fixed it.
  2. Under the Documentation section of the reviewer template it asks for Metadata. I wasn't able to check this off because I wasn't able to find this information if it exists.
    • We have a Contributors file in the repo that contain our names & e-mail addresses.
  3. As I mentioned, I tested the package out on different datasets in addition to those in the README. I noticed that if a user makes a simple barchart of a single variable the color theme will not change. This seems reasonable since aesthetically there is no need to color every single bar a different color. The README barchart changes colors because it is plotting 2 different years. I'm not sure if a note about this behaviour should be included in the docstring just to alert users to this behavior if just in case this isn't what they are expecting.
    • Thank you for pointing this out, but I would say a user might not find many things in our package that he is expecting. To convey this information to the user, I have included a link to the package README in the docstring.
  4. Since each theme is only 5-6 colors, plots that have more than this number of categories can not be uniquely identified by color. This isn't unique to your package as I've discovered this using other palette tools in the past. But perhaps this should be explicitly noted to the user? This is probably too nitpicky and I'm not even sure this is necessary since I've seen this issue before with other palettes in ggplot and no mention was made about this.
    • This a valid concern a user might face when dealing with categories more than what the visual identity of a university provides. For this reason, we have added a link to the university's visual identity in the docstring of every function.
  5. Currently it seems the build is failing on both versions of Windows but this didn't effect me. - I have noticed this too, and mainly it fails while uploading coverage report to Codecov. I don't think there is any other reason for it to fail.
  6. There is a tiny typo in the first line of the first paragraph in the README under hueniversitypy in the Python ecosystem. (the word alongside is misspelled)
    • Fixed it.

Once we have addressed feedback from all the reviewers, we will create a new release for the package and update you on that.

Regards, Shivam Verma

evelynmoorhouse commented 4 years ago

Hi @ttimbers

Thanks for the suggestions for the package! We have addressed the first two points. We moved the Read the Docs link further up in the README.md above the usage so it is obvious, and we added how the package should be cited including bibtex.

For your final three points, we agree they are all great suggestions! The ability to build a theme would allow this package to be useful globally for all universities and colleges, especially if we were to create a function to allow a user to do it. For now it is out of our scope, but is something to consider for future improvements.

Thanks! -hueniversitypy team

evelynmoorhouse commented 4 years ago

Hi all, Please see the attached link for version 1.1.1 where the changes mentioned above have been implemented.

ttimbers commented 4 years ago

Nice work folks! Very cool to see these improvements to your package!