Skip to content

Save proposed MCMC steps - #88

Open
manueltabreu wants to merge 5 commits into
snoplus:masterfrom
manueltabreu:proposed-steps
Open

manueltabreu wants to merge 5 commits into
snoplus:masterfrom
manueltabreu:proposed-steps

Conversation

@manueltabreu

Copy link
Copy Markdown
Collaborator

To resolve the issue opened by Daniel a few months ago.

Comment thread src/optimise/MCMC.cpp Outdated
}else{
fSamples.FillTree(fCurrentStep, fCurrentVal, accepted);
}
fSamples.Fill(fCurrentStep, fCurrentVal, accepted);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

the fChain gets initialised in InitialiseHistograms, which only gets called in Fill(). So I think we need fSamples.Fill(fCurrentStep, fCurrentVal, accepted); to be before the if statement. alternatively could put the same if(!fInitialised) inside FillTree, but I think this is simpler

Looks great otherwise!

@willp240
willp240 marked this pull request as draft September 30, 2026 13:06
@willp240
willp240 marked this pull request as ready for review September 30, 2026 13:06
Comment thread src/optimise/MCMC.cpp Outdated
Comment thread test/unit/MCMCTest.cpp Outdated
Comment thread test/unit/MCMCTest.cpp Outdated
@willp240

Copy link
Copy Markdown

I think if you just address those new inline comments that should get the new test running and passing.

But then it would be great to extend that test to use the new functionality by enabling SetSaveProposedSteps(true), retrieving the saved chain, and checking that it contains the expected number of entries

In an ideal world we'd also check it contains the exact right values (proposed or accepted), but it's probably tricky to do that in a test as the step proposals aren't deterministic

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.

2 participants