Open tejasph 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: ~ 3 hours
Overall, very interesting idea for a package and really well done! Just a few notes below:
[x] Although the usage section has comprehensive examples, I think you need to provide the output (i.e. the resulting plot) to satisfy the "Vignette" criteria.
[x] I enjoyed the "Package Walkthrough" section that documents each function contained in the altairexpress
package. However, the README
seemed a little repetitive in that there were multiple sections where there were usage examples, as you have both the package walkthrough demos and the usage section. I'm not sure how these differ (the same time series example is even used), so I would either amalgamate them or differentiate them more.
[x] I did not put a check beside the example section because when trying to implement the examples on my local machine I could not run the examples from just copying and pasting (see further notes in functionality).
[ ] For the 'Metadata' section, I would just include the emails beside each package developer in the README
and CONTRIBUTORS
[ ] All of the important elements of the README
are present, although the order is not quite the same as outlined above.
Installation worked!
[x] When I tried to run the usage examples in my jupyter lab they did not work from just copying and pasting. I think they just had a few typos - such as not putting gdpPercap
in quotes and capitalizating the 'c' in gdpPercap
. Also, when importing altairexpress.hist as hist
the function should be hist()
, rather than altairexpress.hist()
or else it throws an error. There were a lot of typos in the other functions as well, so I would just go through and proofread these. The docstrings were very helpful in figuring out how to use the hist
and fourier_transform
functions, though. However, I had to look at the function code to learn how to run the scatter
and ts
functions.
[x] Once I managed to run the functions, they worked really well and did everything they said they would! This is a bit nitpicky, but when running the different functions I noticed a few inconsistencies between the styles of visualizations. For instance, some plots had titles while others did not. I would just do a little touch up to ensure that all plots contain the same elements.
All tests passed!
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: 3
Read the Docs
[x] "From Sources" installation links are broken (not certain if the links provided here are correct).
[x] "Usage" contains no examples.
README
[x] "Usage" is a bit confusing - I'd like to see some explanations in this section.
[x] no fourier_transform
working example
[x] gghist()
but hist()
is called
[x] scatter_express()
but scatter()
is called
[x] ts_plot()
but ts_alt()
is called
[x] It may be useful to provide an example of the outputs of each function in the "Usage" or "Package Walkthrough"
[x] Include a description of the returns of each function in the function descriptions
[ ] not all package dependencies are included; my installation failed because I did not have statsmodels
, vega-datasets
, and gapminder
installed
[ ] altair
dependency version not correct (>4 required)
Installation
[x] installation failed when running the command in the README, should instead be:
pip install -i https://test.pypi.org/simple/ altairexpress
Functionality
hist
[x] Package Walk Through has incorrect call after import, should be:
hist(gapminder, 'lifeExp')
[ ] the text output of certain "variables" are overlaid and are illegible
make_scatter
[x] Package Walk Through has incorrect import call and function call, should be:
... import make_scatter
make_scatter(...)
[x] Package documentation on Read the Docs indicates a log transformation parameters of x_logtransform()
and y_logtransform()
, however, actual function parameters do not include the "log"
ts_alt
[x] Package Walk Through has incorrect import call, should be:
[x] from altairexpress.ts import ts_alt
fourier_transform
performs as documented Testing
[ ] Some of your test functions have quite a bit of commented out code - if this is unnecessary or broken code, there is no need to include it.
General Comments
[ ] I like the idea of streamlining some of EDA process in some easy to implement functions. The make_scatter
function is particularly effective and makes the hist
function somewhat redundant (instead of both functions, perhaps overlay the summary statistics from hist
on to make_scatter
). Further, the ability to log transform the data is quite useful.
[ ] The ts_alt
function is an effective visual, but if the data is being decomposed, I would like to have access to this transformed data. In addition to returning the plots, perhaps consider also returning a dictionary of trend, seasonal, and residual components.
Overall, I enjoyed working my way through your package; I think there are some improvements that can be made, some of which are suggested, but the practical use of the package is evident.
Thanks for the insightful feedback @katieb1 and @evhend. We will work through all your suggestions.
From Evhen's comments I have implement the following changes in our latest version:
Corrected the hist()
import call in the Readme Package Walkthrough
Corrected the make_scatter
import call in the Readme Package Walkthrough Section
In Read the docs, corrected a miss-specified in the make_scatter
function. The defined variables now line up accurately with variables found inside the function.
Corrected incorrect import, changing ts
to ts_alt
fixed installation command; the install still fails for me though with the following errors:
ERROR: Could not find a version that satisfies the requirement datetools<2.0,>=1.1 (from altairexpress) (from versions: none)
ERROR: No matching distribution found for datetools<2.0,>=1.1 (from altairexpress)
According to someone on Slack, we can't account for these errors unless we have a setup.py or a requirements.txt file. Instead, the user must upgrade the packages that are being referred to in the error. We are still trying to find a fix for this.
And the following is our latest release which has addressed the feedback above: v 1.1.24
All comments that have been checked off under Katie's review that has been addressed. Please see her review comments on details of the action items.
Submitting Authors: Tejas Phaterpekar(@tejasph), Lesley Miller (@aromatic-toast), Jack Tan (@thejacktan), Wenjiao Zou(@zouwenjiao) Package Name: altairexpress One-Line Description of Package: Provides efficient EDA plots in Altair Repository Link: https://github.com/UBC-MDS/altairexpress Version submitted: V1.1.16 Editor: Varada Kolhatkar (@kvarada) Reviewer 1: Evhen Dytyniak (@evhend) Reviewer 2: Katherine Birchard (@katieb1) Archive: TBD
Version 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.
@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
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