Open roycezhou opened 3 years ago
Hello, please find my package review below:
The package includes all the following forms of documentation:
[x] A statement of need clearly stating problems the software is designed to solve and its target audience in README
[ ] Installation instructions: for the development version of package and any non-standard dependencies in README
pip install -i https://test.pypi.org/simple/ instaeda
are different from those on readthedocs.io: pip install -u instaeda
, which caused an error: no such option: -u
$ git clone git://github.com/jufu/instaeda
Cloning into 'instaeda'...
fatal: remote error:
Repository not found.
git clone https://github.com/UBC-MDS/instaeda_py.git
[ ] Vignette(s) demonstrating major functionality that runs successfully locally
[x] Function Documentation: for all user-facing functions
plot_basic_distributions
has unclear wording in the description of the return type[x] Examples for all user-facing functions
[x] Community guidelines including contribution guidelines in the README or CONTRIBUTING.
[x] Metadata including author(s), author e-mail(s), a url, and any other relevant metadata e.g., in a setup.py
file or elsewhere.
The package meets the readme requirements below:
The README should include, from top to bottom:
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:
plot_basic_distributions
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:
Thanks for the submission. This looks like a useful package that just needs some minor polishing to be ready to go. I left thorough notes inline (above) but have summarized the main points here:
plot_intro(penguin_df)
It's not clear how to interpret the "All Missing Columns" entry in the plot and why it is showing near 240%. Could you explain what this quantity is meant to represent?
See notes above on Installation. There are a few little details to tidy up
git clone git://github.com/jufu/instaeda
. Should this be instaeda_py
instead?See notes above on Vignettes.
See notes above on Documentation.
plot_basic_distributions
has unclear wording in the description of the return type. It looks like it might be mid-edit.Please let me know if you have questions or want to discuss anything further.
Please check off boxes as applicable, and elaborate in the 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:
Reviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole. The package structure should follow general community best-practices. In general please consider:
-[x] Installation: Installation succeeds as documented.
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:
2.5
Hi Justin, Selma, Zeliha, Siqi, Thanks for putting this the package together. I had fun playing around with the instaeda_py package and they are definitely going to make our data exploration more convenient.
While I was playing with the package, I noted a few things down below, and hope it could help to improve the overall experience of the package more.
I tried following package installation instructions following your README file, but sadly it fails in an empty Python 3 environment, which gave me the exception below:
ERROR: Cannot install instaeda==0.1.0, instaeda==0.1.1, instaeda==0.1.2, instaeda==0.1.6, instaeda==0.1.7 and instaeda==0.1.8 because these package versions have conflicting dependencies.
The conflict is caused by: instaeda 0.1.8 depends on vega-datasets<0.10.0 and >=0.9.0 instaeda 0.1.7 depends on vega-datasets<0.10.0 and >=0.9.0 instaeda 0.1.6 depends on vega-datasets<0.10.0 and >=0.9.0 instaeda 0.1.2 depends on vega-datasets<0.10.0 and >=0.9.0 instaeda 0.1.1 depends on vega-datasets<0.10.0 and >=0.9.0 instaeda 0.1.0 depends on vega-datasets<0.10.0 and >=0.9.0
> However, it seems I could install it after adding the `--extra-index-url` option.
pip install -i https://test.pypi.org/simple --extra-index-url https://pypi.org/simple instaeda
#### Tests
> I really like how many test cases you created and they seem to be in large detail.
> I tried to clone the repo to my laptop and run the pytest locally. The command I used was `poetry install` then `poetry run pytest`, then I ran into an error below:
ModuleNotFoundError: No module named '_cffi_backend'
> I'm not sure if it is an issue from my environment as I created a new environment to test this...:(
#### Function Documentation
> I really loved your function documentations. The examples in the docstrings are very clear to understand.
#### Documentation
> I only see Justin’s name in the copyright info. I wonder if this should be all members of the team.
> It seems you don't have a license discussion issue...:/
> The usage part on the [readthedocs](https://instaeda.readthedocs.io/en/latest/usage.html) is not there yet.
Submitting Author:
Package Name: instaeda One-Line Description of Package: Quick and easy way to clean data and build exploratory data analysis plots Repository Link: instaeda_py Version submitted: 0.1.6 Editor: TBD
Reviewer 1: TBD
Reviewer 2: TBD
Archive: TBD
Version accepted: TBD
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.
@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][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