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

WIP MCMC with transforms Interface #409

Closed
wants to merge 39 commits into from
Closed

WIP MCMC with transforms Interface #409

wants to merge 39 commits into from

Conversation

waldie11
Copy link
Collaborator

@oschulz
Copy link
Member

oschulz commented Jun 30, 2023

Can you check the

Base.Meta.ParseError("parsing error in src/samplers/transformed_mcmc/struct_list.jl:34: missing comma or } in argument list")

CI error on Julia 1?

@codecov
Copy link

codecov bot commented Jul 3, 2023

Codecov Report

Patch coverage has no change and project coverage change: -5.76% ⚠️

Comparison is base (78e9339) 55.06% compared to head (9cb2625) 49.31%.

❗ Current head 9cb2625 differs from pull request most recent head b6565c3. Consider uploading reports for the commit b6565c3 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #409      +/-   ##
==========================================
- Coverage   55.06%   49.31%   -5.76%     
==========================================
  Files         116      131      +15     
  Lines        5619     6323     +704     
==========================================
+ Hits         3094     3118      +24     
- Misses       2525     3205     +680     
Files Changed Coverage Δ
src/BAT.jl 100.00% <ø> (ø)
src/samplers/transformed_mcmc/chain_pool_init.jl 0.00% <0.00%> (ø)
src/samplers/transformed_mcmc/mcmc_algorithm.jl 0.00% <0.00%> (ø)
src/samplers/transformed_mcmc/mcmc_convergence.jl 0.00% <0.00%> (ø)
src/samplers/transformed_mcmc/mcmc_iterate.jl 0.00% <0.00%> (ø)
src/samplers/transformed_mcmc/mcmc_sample.jl 0.00% <0.00%> (ø)
src/samplers/transformed_mcmc/mcmc_sampleid.jl 0.00% <0.00%> (ø)
src/samplers/transformed_mcmc/mcmc_stats.jl 0.00% <0.00%> (ø)
...rs/transformed_mcmc/mcmc_tuning/mcmc_noop_tuner.jl 0.00% <0.00%> (ø)
...sformed_mcmc/mcmc_tuning/mcmc_proposalcov_tuner.jl 0.00% <0.00%> (ø)
... and 7 more

... and 64 files with indirect coverage changes

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

@oschulz
Copy link
Member

oschulz commented Jul 6, 2023

I've updated this to use the new BATContext. "transformed_example.jl" doesn't fully run through, but I'm not sure if that's related.

@oschulz
Copy link
Member

oschulz commented Jul 6, 2023

TransformedAdaptiveMHTuning in the example works now, but

r_hmc = @time BAT.bat_sample_impl(posterior, MCMCSampling(mcalg=HamiltonianMC(), nchains=4, nsteps=4*20000), context)

in the example doesn't work yet, but that's to be expected currently, right?

I think BATContext-wise this is fine now.

@Cornelius-G
Copy link
Collaborator

TransformedAdaptiveMHTuning in the example works now, but

r_hmc = @time BAT.bat_sample_impl(posterior, MCMCSampling(mcalg=HamiltonianMC(), nchains=4, nsteps=4*20000), context)

in the example doesn't work yet, but that's to be expected currently, right?

Hm, for me this line works. As it should, because that is just the "good old" HMC smapling, not using the new transformations.

@oschulz
Copy link
Member

oschulz commented Jul 6, 2023

Ah - well, in any case, I think the adaption to BATContext is complete.

@@ -58,7 +58,7 @@ end

# this function is called once after each tuning cycle
g_state = nothing
function tuning_update!(tuner::TransformedProposalCovTuner, chain::MCMCIterator, samples::DensitySampleVector)
function tuning_update!(tuner::TransformedProposalCovTuner, chain::TransformedMCMCIterator, samples::DensitySampleVector)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we need to revert this change, because the CI tests fail due to this line.
TransformedMCMCIterator is a subtype of MCMCIterator. Dispatch should be fine just using the tuner argument.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, we can revert it. But why does it cause errors? For a TransformedProposalCovTuner the chain must be a TransformedMCMCIterator, right?

@oschulz
Copy link
Member

oschulz commented May 21, 2024

@Micki-D needs to be updated to API changes.

@oschulz
Copy link
Member

oschulz commented Jun 5, 2024

Closing in favor of #441

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