Conversation
reneeli411
commented
Oct 14, 2018
reneeli411
commented
- Update README to include Matplot lib requirement
* Update README to add Matplotlib requirement
* Update README to include Matplotlib requirement
* Update README to include Matplotlib requirement
reneeli411
requested review from
claban3,
jonathancstroud,
mkbwang and
wesleytian
as code owners
jonathancstroud
left a comment
jonathancstroud
left a comment
There was a problem hiding this comment.
Thanks Renee! I reverted your changes which were pushed directly to the master branch (this isn't supposed to happen) and reopened the PR. I have requested some changes.
| ``` | ||
|
|
||
| 6. Install the required packages. | ||
| 6. Install the required packages. Note: need to have matplotlib 3.0.0 installed. |
There was a problem hiding this comment.
Please add the requirement to requirements.txt directly
There was a problem hiding this comment.
It looks like there's an issue with having matplotlib in requirements.txt: https://stackoverflow.com/questions/11797688/matplotlib-requirements-with-pip-install-in-virtualenv
| print("Percent of total sessions using a particular device that generated revenue: \n", | ||
| explore_utils.find_percent_device_uses_generating_revenue(data)) | ||
|
|
||
| # Plots (1) monthly and (2) yearly histograms of number of site visits, |
There was a problem hiding this comment.
Please add a command-line argument (or multiple) that allows the user to select whether or not to display these graphs, and also whether or not to save them to disk.
explore.py
Outdated
| axs[3].set_title('Percent purchase') | ||
| axs[3].set_ylabel('Percent') | ||
|
|
||
| plt.show() |
There was a problem hiding this comment.
Please also allow the option for the graphs to be saved to disk
naitian
left a comment
naitian
left a comment
There was a problem hiding this comment.
Just a minor code style thing. Otherwise, once you make @jonathancstroud's changes, this LGTM.
| ``` | ||
|
|
||
| 6. Install the required packages. | ||
| 6. Install the required packages. Note: need to have matplotlib 3.0.0 installed. |
There was a problem hiding this comment.
It looks like there's an issue with having matplotlib in requirements.txt: https://stackoverflow.com/questions/11797688/matplotlib-requirements-with-pip-install-in-virtualenv
explore_utils.py
Outdated
|
|
||
| daily_count = df.groupby('datetime').datetime.count() | ||
| daily_revenue_count = df.groupby('datetime').revenue.count() | ||
| daily_percent = daily_revenue_count/daily_count |
There was a problem hiding this comment.
Code style, put spaces between operators.
| daily_percent = daily_revenue_count/daily_count | |
| daily_percent = daily_revenue_count / daily_count |
reneeli411
requested review from
erisllangos and
sethsaperstein
as code owners
erisllangos
left a comment
erisllangos
left a comment
There was a problem hiding this comment.
- style error with spacing fixed
- saving figure option done
- requirement file changes?
|
LGTM |
5 participants
