- Notifications
You must be signed in to change notification settings - Fork277
Add hacker-stats notebooks#98
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered byReviewNB |
ericmjl left a comment
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.
Cool beans, thanks@hugobowne!
I've dropped a few comments here that should help with organizing things nicely.
The only one I couldn't drop a comment for is the filenames for the notebooks; could you give them a shorter names, perhaps "01-simulated-probability.ipynb" and "02-parameter-estimation.ipynb"? Doing so makes managing themkdocs.yml file navigation section just that teeny bit easier.
| @@ -0,0 +1,74 @@ | |||
| import pandas as pd | |||
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.
I think the entiredata.py can be placed inside the project'ssrc/bayes_tutorial/data.py; there's some data loaders already in there. Inside the notebooks, it should be a small change to get the data imported:
frombayes_tutorial.dataimport<thenecessaryfunctions>
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.
HBA to move all data imports intosrc/bayes_tutorial/data.py (w/out duplication).
Edit import calls in NBs
| @@ -0,0 +1,5 @@ | |||
| axes.spines.left : True # display axis spines | |||
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.
I like the style sheet; let's get this one used across the entire series.
According to theMatplotlib docs, we can reference it from a single source of truth using the following pattern:
importmatplotlib.pyplotaspltplt.style.use('./images/presentation.mplstyle')
I'd like to propose that we use the style sheet by doing the following:
- Place the
matplotlibrcfile in the project root directory. - Reference it at the top of every notebook (I'll take care of that for the other notebooks I've written), using the following pattern:
importmatplotlib.pyplotaspltfrompyprojrootimporthereplt.style.use(here()/"matplotlibrc")
What do you think,@hugobowne?
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.
HBA to movematplotlibrc intoconfig/matplotibrc
and load at top of both NBs.
| @@ -0,0 +1,19 @@ | |||
| import numpy as np | |||
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.
Theutils.py module can probably be moved to the pathsrc/bayes_tutorial/utils.py, which would allow us to use it across all of the notebooks.
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.
HBA to do
| def despine(ax): | ||
| ax.spines['right'].set_visible(False) | ||
| ax.spines['top'].set_visible(False) |
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.
With thematplotlibrc file in place, do we need this function? If so, we keep it.
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.
I don't think I wrote any of this 👯♂️
| def despine_traceplot(traceplot): | ||
| for row in traceplot: | ||
| for ax in row: | ||
| despine(ax) No newline at end of file |
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.
Ditto with this function; if we havematplotlibrc, is this function still needed? If so, we keep it.
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.
👯♂️
Uh oh!
There was an error while loading.Please reload this page.
@ericmjl as discussed in#97.
These NBs run programmatically but there's the small issue of data access: I think I need to mimic something you did and use some utility functions in the NBs to load the data?
We can discuss tomorrow but it may be worth adding some instructions to#97 so others can reproduce, if and when necessary.
Cool!