-
Notifications
You must be signed in to change notification settings - Fork 101
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
Fix MCMC_walkthrough.ipynb
#1679
Conversation
Strangely, when I run it locally I don't get the same errors and I don't get a good phase-o-gram |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1679 +/- ##
=======================================
Coverage 68.42% 68.42%
=======================================
Files 104 104
Lines 24339 24339
Branches 4346 4346
=======================================
Hits 16653 16653
Misses 6599 6599
Partials 1087 1087 ☔ View full report in Codecov by Sentry. |
I thought I fixed the second error that appears by the phaseogram, but it's still there in the RTD. |
So I fixed the syntax errors and now don't get good phase-o-grams. But at least now I see consistent results. |
So I realized that the reason the phaseograms looked bad was because it was replacing F1 with the phase in the fit. It computed:
but then set |
Also, the second example discussed using a phase term in the likelihood (and hard a hard bound on the prior for that). But I don't see it in the model. With the phase prior removed it now doesn't give |
The absolute phase information for the fit used to be tacked on invisibly at the end of the parameter vectors (that's why we loop up to |
Do you think that needs to be changed for this notebook? Or maybe we can merge this notebook back in and put in a new issue on adding the phase info back? |
This looks good to me. Shall I merge this? |
Yes, please. |
This notebook runs, although I'm not sure if the results are sensible/as intended.