-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Examples of pandas and plotly in jupyter notebook #56
base: master
Are you sure you want to change the base?
Conversation
Thanks for the contribution! There are some comments that IMO should be addressed before it gets to a "ready to merge" state:
On the other hand, IMO this should be independent of pipenv. I would expect people to use this Jupyter right after doing "pip install opensir". So, neither "plotly" nor "pandas" should be in the Pipfile (although #35 added the dependency because it's intended to be shown as a Sphinx page). Also, I would try to limit the scope of this file to one of these:
What do you think? PS: Regarding comments 1-3, if you have any questions don't hesitate to ask @felipehuerta17 , @leandrolanzieri , @sasalatart or me. |
Hi all, Thanks for the very well and polished contribution @gwenzel ! Besides from the TI aspects, this looks a great example on how:
Observations
They look excellent and they are within the scope of open-sir, not only "plotly", because you are merging your modelling thinking with a data visualization approach. 2.1) Figure 2.1 looks great and it's much better displayed than using matplotlib. I think this should be a separate tutorial, and have crosslinks with SIR.ipynb. As the hello-world is SIR.ipynb, after the sensitivity analysis we can provide a link on SIR.ipynb to this notebook to show how to create more compelling visualizations. In a similar way, before importing data provide a brief description of the SIR model (no diff eqs), and provide a link to SIR.ipynb and the documentation so the reader can get more context. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great contribution and first commit!
To be ready to merge:
- Take onboard the specific comments on the different parts of the code.
- Provide a narrative tutorial on "how to use plotly to increase the impact of the results that can be generated using open-sir"
- With the new repository, you may need to do from opensir.models import SIR,SIRX
- Let me know if you need help on doing the TI stuff with the commits. We can do this together in 5-10 minutes after you deal with the other changes. I tried to solve similar comments by my own in the past and took a lot of time for the first time and built frustration.
Pipfile
Outdated
@@ -14,6 +14,8 @@ matplotlib = "*" | |||
scipy = "*" | |||
jupyter = "*" | |||
sklearn = "*" | |||
pandas = "*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be updated when you update your master repository and rebase this branch :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"pandas" was already added, so this commit can be dropped
Pipfile
Outdated
@@ -14,6 +14,8 @@ matplotlib = "*" | |||
scipy = "*" | |||
jupyter = "*" | |||
sklearn = "*" | |||
pandas = "*" | |||
plotly = "*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jia200x @gwenzel here we need to make a decision whether we make plotly a dependency of opensir, or we write in the notebook that plotly is required and the user has to install it using pip install plotly in order to run the notebook.
At the moment, I wouldn't be against of making plotly a dependency as the plotly figures created by @gwenzel may constitute in the future a function of model.plot().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jia200x @gwenzel here we need to make a decision whether we make plotly a dependency of opensir, or we write in the notebook that plotly is required and the user has to install it using pip install plotly in order to run the notebook.
As described above, the user of this Jupyter is the End User, not the developer user.
There are a lot of problems of using Pipenv for end users (e.g if the user doesn't have Python 3.7, it won't be able to run opensir).
At the moment, I wouldn't be against of making plotly a dependency as the plotly figures created by @gwenzel may constitute in the future a function of model.plot().
If the model.plot uses plotly, then that PR should add the dependency. Adding spare dependencies is not recommended because it adds more "moving parts" if not used
3/26/2020,136 | ||
3/27/2020,165 | ||
3/28/2020,209 | ||
3/29/2020,241 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this and the idea of having a datasets folder
test_pandas_plotly.ipynb
Outdated
"# Convert into seconds\n", | ||
"tf_long = long_term_days-1\n", | ||
"sol_long = my_SIR_fitted.solve(tf_long, long_term_days).fetch()\n", | ||
"N_S_long = sol_long[:,1]*P\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Substitute
N_S_long = sol_long[:,1]*P
for
N_S_long = sol_long[:,1]
As now the output of sol is in number of people.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same applies for line 145 and 146
Previous comments were taken into account and a new commit was made: e2ac3ea. |
Description:
A new jupyter notebook was added, showing how to import data from pandas, and how to plot data using the plotly packages. The data and examples are the same as in the initial notebook written by @felipehuerta17 .
List of changes