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

Save gradients #351

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Save gradients #351

wants to merge 4 commits into from

Conversation

JaimeRZP
Copy link
Member

Hi @sethaxen!

This should do what you were suggesting if you use the internal sampling method of AHMC and set drop_warmup=false and keep_gradient=true.

let me know if it works!

src/sampler.jl Outdated Show resolved Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@torfjelde
Copy link
Member

Generally speaking, it's not great to have the return-value change based on a keyword argument , as it leads to type-instabilities 😕 In this scenario, it's probably the not the worst (sample is generally going to be the outermost caller anyways), but it's still not great style.

IMO what we should do here is to

  1. Move AdvancedHMC.jl completely over to the AbstractMCMC.jl interface, i.e. we have step function that takes in a state containing all the necessary information.
  2. Then the gradients can easily be extracted through a callback which just extracts this information from the state.

(1) will also just be a huge gain in general, as it will make things much more modular:)

@torfjelde
Copy link
Member

Arguably this PR is not needed anymore, since we now have AbstractMCMC.sample with callback support (from which the gradient can be extracted).

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.

3 participants