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

[WIP] Review changes to survival_analysis.ipynb #153

Merged
merged 5 commits into from
May 6, 2021

Conversation

olgadk7
Copy link
Contributor

@olgadk7 olgadk7 commented May 4, 2021

Wanted to submit the incorporated changes recommended in #96: all but the convergence one (looking into it this week). Had to start another branch as the original one had irresolvable conflicts (guessing because of me not submitting often).

Closes pymc-devs/pymc#4603, addresses #128

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@olgadk7
Copy link
Contributor Author

olgadk7 commented May 4, 2021

@MarcoGorelli, @OriolAbril Regarding the convergence issue, increasing the target_accept does the trick: the number divergences is cut in half, r_hat is 1, the sampler converges, the chains in the forest plot lay snug, and the posterior looks almost exactly the same as in the original notebook. Is that enough for the job at hand or should we reparameterize?

@review-notebook-app
Copy link

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2021-05-05T20:50:16Z
----------------------------------------------------------------

As Marco pointed out recently and added to the style guide, we should specify which error to catch, my original advise was not best practices. Here is the link: https://github.com/pymc-devs/pymc3/wiki/PyMC3-Jupyter-Notebook-Style-Guide


@review-notebook-app
Copy link

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2021-05-05T20:50:17Z
----------------------------------------------------------------

I would delete the commented line (if you haven't already)


Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Increasing the target_accept is more than enough for our purposes here. I commented two very minor nits, it could be merged as is once you push the converged results too.

This is already great, thanks for the PR! I hope it has been an enriching and pleasurable experience

@olgadk7
Copy link
Contributor Author

olgadk7 commented May 6, 2021

Yes, enriching and pleasurable, thank you! Contributing is definitely one of the best way to learn, and with your help, support, and patience this was an invaluable experience. I’ll be on the lookout for opportunities to continue participating :)

@OriolAbril OriolAbril merged commit f3e8614 into pymc-devs:main May 6, 2021
@olgadk7
Copy link
Contributor Author

olgadk7 commented May 10, 2021

Curious, when do the notebooks / code base update to reflect the merged changes? What does it depend on? Thank you.

@MarcoGorelli
Copy link
Contributor

Great job @olgadk7 !

The website'll be updated with the next release of PyMC3 (ATM it's not updated after each commit to this repo - but perhaps it should be!)

@OriolAbril
Copy link
Member

The website'll be updated with the next release of PyMC3

And this is a completely manual process which we sometimes forget or do significantly later than the release. 😅

(ATM it's not updated after each commit to this repo - but perhaps it should be!)

Indeed, it would be amazing to have both release and dev version docs, with the dev version being updated with every commit here, I believe we already have an issue for 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.

Typos in the survival_analysis notebook
3 participants