Skip to content
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

Bayesian Hierarchical Stacking - well switching case study #226

Closed
wants to merge 9 commits into from

Conversation

MarcoGorelli
Copy link
Contributor

@MarcoGorelli MarcoGorelli commented Sep 13, 2021

Description

A Python implementation of the first case study from https://arxiv.org/pdf/2101.08954.pdf

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@MarcoGorelli MarcoGorelli marked this pull request as draft September 13, 2021 16:49
@MarcoGorelli MarcoGorelli marked this pull request as ready for review September 18, 2021 11:21
@MarcoGorelli MarcoGorelli marked this pull request as draft September 18, 2021 11:21
@MarcoGorelli MarcoGorelli marked this pull request as ready for review September 19, 2021 07:48
@OriolAbril
Copy link
Member

OriolAbril commented Oct 2, 2021

haven't really read the notebook yet, only 1 minute skim to check formatting mostly, I'll read the notebook whenever I have time, I love that we have a notebook for this paper 😍 . Some quick comments

The initial image doesn't render on the website, I suspect the reason for that is that raw html is not modifed by sphinx and the naive markdown/jupyter preview uses a different home folder than the sphinx/website renders do. It might be possible to get myst to modify raw html to fix this issue, I'd guess it will but I don't know. Whatever the case, https://myst-parser.readthedocs.io/en/latest/syntax/syntax.html#images and references therein will either explain how to get myst to handle this html image case or explain the recommended way of handling that. I think the ideal case would be to use the image directive and add a caption to the image.

The theme already generates a table of content automatically at the right sidebar, there is no need for a manual one too. You should also double check the title levels. While we are using markdown, sphinx only supports consecutive title levels which means that the main notebook title should be level 1, intro/eda/preparation... level 2 all of them, and the subsections should be level 3. Somehow this message Non-consecutive header level increase; 1 to 3 [myst.header] got to the notebook exerpt at https://pymc-examples--226.org.readthedocs.build/en/226/blog/tag/pymc3model.html.

There is one somewhat working citation (bibtex missing journal error only) and one non working myst citation. It looks like you the wrong one is using rST instead of myst syntax. Also, to ease thing going forward, it would be great to start the reference ids with the first author's last name and sorting them alphabetically so when it's a giant file we can still find which references are there and avoid duplication. And lastly, even though the reference is not working inline, the reference still appears in the bibliography, so it looks like the explanation at https://github.com/pymc-devs/pymc/wiki/PyMC3-Jupyter-Notebook-Style-Guide#references is not clear enough. I'll try to explain better here, if you can then fix the wiki that would be great. All references cited within the text with {cite:t/p/...} will automatically appear in the bibliography, this is what the filter takes care of. References only need to be listed manually within the bibliography directive if they are not cited inline but we still want them to appear in the bibliography.

@MarcoGorelli
Copy link
Contributor Author

Thanks @OriolAbril for your feedback - I'm gonna be on holiday for the next two weeks, so marking as draft for now, will pick it up again when I'm back

@MarcoGorelli MarcoGorelli marked this pull request as draft October 2, 2021 13:04
@OriolAbril OriolAbril marked this pull request as ready for review December 16, 2021 15:19
@fonnesbeck
Copy link
Member

Nit: put variable names in the list before cell 4 in backticks to be consistent with what you have below.

@fonnesbeck
Copy link
Member

Is there any context as to why those 6 models were selected for comparison? If so, it would be helpful to add a line or two addressing this.

@MarcoGorelli
Copy link
Contributor Author

There's no context unfortunately, and to be honest I think a more diverse set of models might have been more illustrative (e.g. on Kaggle the boost that people get from combining models is when the models are different, e.g. a GBDT and a neural network)

@fonnesbeck
Copy link
Member

Remove %matplotlib inline statement. This is no longer needed in Jupyter for inline plotting.

@fonnesbeck
Copy link
Member

Just noticed that all of this is in PyMC3/Theano. Since v4's release is imminent, it should be ported over.

@fonnesbeck
Copy link
Member

@MarcoGorelli will you be able to port this to v4, or do you want to hand it off?

@MarcoGorelli
Copy link
Contributor Author

Hey - apologies, haven't had a chance to get back to this

If someone wants to take over, they're more than welcome to, that'd be great, thanks

Else I'll get back to it when I have a chance

@MarcoGorelli
Copy link
Contributor Author

Else I'll get back to it when I have a chance

probably not gonna have a chance anytime soon - closing then, and I'm sorry about this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants