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

Bringback old getparams #2086

Closed
wants to merge 1 commit into from
Closed

Bringback old getparams #2086

wants to merge 1 commit into from

Conversation

JaimeRZP
Copy link
Member

@JaimeRZP JaimeRZP commented Sep 22, 2023

getparams(t) = t.θ is extremely useful to connect external samplers to Turing while

# NOTE: Only thing that depends on the underlying sampler.
# Something similar should be part of AbstractMCMC at some point:
# https://github.com/TuringLang/AbstractMCMC.jl/pull/86
getparams(transition::AdvancedHMC.Transition) = transition.z.θ
getstats(transition::AdvancedHMC.Transition) = transition.stat

getparams(transition::AdvancedMH.Transition) = transition.params

getvarinfo(f::DynamicPPL.LogDensityFunction) = f.varinfo
getvarinfo(f::LogDensityProblemsAD.ADGradientWrapper) = getvarinfo(parent(f))

setvarinfo(f::DynamicPPL.LogDensityFunction, varinfo) = Setfield.@set f.varinfo = varinfo
setvarinfo(f::LogDensityProblemsAD.ADGradientWrapper, varinfo) = setvarinfo(parent(f), varinfo)

get properly pushed into abstractmcmc.

@JaimeRZP
Copy link
Member Author

@torfjelde Could you quickly have a look? It is just one line of code that I need to connect some external samplers to Turing.

@JaimeRZP JaimeRZP requested a review from torfjelde September 22, 2023 15:01
@devmotion
Copy link
Member

Maybe it would be better to finish TuringLang/AbstractMCMC.jl#86?

@codecov
Copy link

codecov bot commented Sep 22, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (8d8416a) 0.00% compared to head (5734dca) 0.00%.

Additional details and impacted files
@@          Coverage Diff           @@
##           master   #2086   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files          21      21           
  Lines        1451    1452    +1     
======================================
- Misses       1451    1452    +1     
Files Changed Coverage Δ
src/mcmc/Inference.jl 0.00% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JaimeRZP
Copy link
Member Author

Maybe it would be better to finish TuringLang/AbstractMCMC.jl#86?

I completely agree that this is ultimately the correct solution.
This is just an extremely quick patch to allow external libraries to connect to Turing.
The latest change broke a lot of connections, this brings back that functionality while we work on the proper solution.

@torfjelde
Copy link
Member

torfjelde commented Sep 23, 2023 via email

@torfjelde
Copy link
Member

Sorry, why can't we just provide the model and use the existing getparams?

@torfjelde
Copy link
Member

And I'm also very much in favor of what @devmotion is suggesting

@JaimeRZP
Copy link
Member Author

Sorry, why can't we just provide the model and use the existing getparams?

Because that's not the signature of getparams used in TuringTransition. See code here

@torfjelde
Copy link
Member

Sure, but there's nothing stopping us from just changing TuringTransition to use this signature, right? It would just be mapping getparams(transition) to getparams(f.model, transition) (and this should of course have been done in the PR that original introduced this new getparams)

@JaimeRZP
Copy link
Member Author

JaimeRZP commented Oct 2, 2023

Sure.
We can now close this PR in favour yours!

@JaimeRZP JaimeRZP closed this Oct 2, 2023
@devmotion devmotion deleted the getparams branch October 2, 2023 11:02
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