-
Notifications
You must be signed in to change notification settings - Fork 17
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
Add getparams
and setparams!!
following AbstractMCMC
v5.5 and v5.6
#103
Conversation
the test is failing because an numerical test (https://github.com/TuringLang/AdvancedMH.jl/actions/runs/11434545096/job/31870916921?pr=103#step:5:353) |
@sunxd3 thansk for the PR. Please feel free to relax the assertation bound a bit. |
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.
The PRNG generates different numbers on different Julia versions even if the seed is set the same, so I guess 1234 was just an unlucky choice and we learnt about it when the tests were run on 1.11 😅
Co-authored-by: Penelope Yong <[email protected]>
Oh, one more thing, should it be 0.9.0 rather than 0.8.4? Since we're introducing new functions. |
I am not certain here: these functions are not exported, but it's true that through |
It's a bit weird, but basically, at least to my understanding, when we haven't reached version 1 yet, we use minor version as what is considered "breaking" and patch version for everything that isn't. Once we have non-zero major version, then we bump minor version when we introduce new (even non-breaking) functionality. Sooo in this case, I say we stick with bumping patch version. |
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.
This PR suffers from the same issues I've raised in AdvancedHMC.jl: gradient information will be out of sync.
This means that if you do something like
AbstractMCMC.step(model, sampler, setparams!!(state, new_params))
you'll use the incorrect gradient in the MH step, resulting in the incorrect chain. I've outlined what I think we should do here: TuringLang/AdvancedHMC.jl#378 (review)
getparams
and setparams!!
following AbstractMCMC
v5.5getparams
and setparams!!
following AbstractMCMC
v5.5 and v5.6
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 good! Just need to remove the BangBang.jl references.
good call! now removed, thanks a lot for reviewing this all these times |
ref: TuringLang/Turing.jl#2367