-
Notifications
You must be signed in to change notification settings - Fork 3
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 66 update exercise04 training for Oct 2023 #71
Conversation
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.
Looks great, really nice flow and explanations - the recent edits have improved it even more. Nice touch to add a link to the CUQIpy article, and changing to GMRF makes very good sense. Only found some tiny things added below. You might consider explicitly mentioning and referring to the online documentation somewhere in the beginning, also mentioning that there will be specific links later on.
Co-authored-by: Jakob Sauer Jørgensen <[email protected]>
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.
Thank you @nabriis for this update and upgrading the notebook for more consistency and clarity! I had just some comments for your consideration.
Co-authored-by: amal-ghamdi <[email protected]>
…istribution vs likelihood
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.
Thank you @nabriis for the updates! LGTM and I am approving. I just left 2 comments about the likelihood and the physical model.
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.
Thank you!
Closes #66
Updated prior distribution to GMRF to patch the example in CUQIpy paper I + many small updates on wording etc.