Open carlinakim opened 4 years ago
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
The package includes all the following forms of documentation:
setup.py
file or elsewhere.Readme requirements The package meets the readme requirements below:
The README should include, from top to bottom:
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:
Estimated hours spent reviewing:
Hi, I really enjoyed reviewing the package. Functions are great and intuitive to use. Here are my thoughts and some recommendations:
General:
pip install -i https://test.pypi.org/simple/ pysketball
nba_ranking
, nba_boxplot
, nba_team_stats
nba_ranking(nba_2018_regular, 'PLAYER' , 'GP', top = 2, ascending = False, fun = 'mean')
is not working Function specific:
nba_scrapper
: Nice user-friendly message for webscraping. Docstring for season_type argument is not updated. I think it would be either regular
or postseason
nba_boxplot
: position
argument and the docstring for position
is a little confusing. I thought I can put string of any columns names.
Error message helps for the case if both position
and teams
have values. I think it would be nice to also include an error message if I input other columns names to the position
argument(for example an error message for position="TEAM"
), or change the position
argument type to boolean
.nba_ranking
: Error message for fun argument is not updated. Currently it says The fun argument should be either var or mean
nba_team_stats
: Nice summary options. It would be nice if you can round the numbers for mean
and std
columns.Overall it's a very good package for NBA enthusiasts! If you have any questions about my review, please feel free to let me know.
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
The package includes all the following forms of documentation:
setup.py
file or elsewhere.Readme requirements The package meets the readme requirements below:
The README should include, from top to bottom:
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:
Estimated hours spent reviewing: 1
Great job, team! I enjoyed playing around with your package. Here are some things I thought are really neat.
chromedriver
. (It gave me some security warning though. Might be worth mentioning that in the documentation.) Here are some comments that might help improve your package.
Installation directions might benefit from some clarification. I had to do the following in order to get it working.
conda install selenium
pip install python-semantic-release
Good that you include the instruction to put the chromedriver
path in the system path variable. I guess you do not mean PATH_TO_CHROMEDRIVER.EXE
in the following command for Linux and MacOS.
export PATH="<PATH_TO_CHROMEDRIVER.EXE>:$PATH"
BTW, on my MacBook Pro, it was installed under /usr/local/bin
by default and I didn't haven't to add the path explicitly.
AttributeError: module 'altair.vegalite' has no attribute 'v4'
nba_ranking
, the 'PLAYER' column is undefined in the scraped CSV and so it gives an error. Replacing it with 'NAME' worked for me. nba_ranking(nba_2018_regular, 'PLAYER' , 'GP', top = 2, ascending = False, fun = 'mean')
In your docstrings, be consistent with the default value of keyword arguments. The usual practice is to write (default = 10) after the data type.
Some of the usage examples in the docstrings are not complete. In many cases, the output is missing. For example, in your docstring of nba_ranking
function, I would give an example with a toy CSV which has similar to your scraped CSV (e.g., NAME, GP columns). Also, showing the output of the example (the dataframe and not the plot) would help.
Submitting Author: Andres Pitta (@AndresPitta ), Kenneth Foo (@kfoofw ), Anand Shankar(@vanandsh ), Carlina Kim (@carlinakim ) Package Name:
pysketball
One-Line Description of Package: A python package to explore ESPN online data through statistics and graphs. Repository Link: pysketball Version submitted: 2.0.0 Editor: Varada Kolhatkar (@kvarada) Reviewer 1: Zhengyang Pan (@zoepan00) Reviewer 2: Varada Kolhatkar (@kvarada) Archive: TBDVersion accepted: TBD
Description
Scope
* Please fill out a pre-submission inquiry before submitting a data visualization package. For more info, see this section of our guidebook.
Explain how the and why the package falls under these categories (briefly, 1-2 sentences):
Who is the target audience and what are scientific applications of this package?
Are there other Python packages that accomplish the same thing? If so, how does yours differ?
If you made a pre-submission enquiry, please paste the link to the corresponding issue, forum post, or other discussion, or
@tag
the editor you contacted: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