Skip to content

Conversation

@penelopeysm
Copy link
Member

Comments in PR!

Comment on lines -9 to -16
# Required for using the slice samplers as `externalsampler`s in Turing
# begin
function Turing.Inference.getparams(
::Turing.DynamicPPL.Model, sample::SliceSampling.Transition
)
return sample.params
end
# end
Copy link
Member Author

Choose a reason for hiding this comment

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

The changes to the external sampler interface are described in https://github.com/TuringLang/Turing.jl/releases/tag/v0.42.0 -- the general aim is that you should not need to overload Turing internal functions (getparams was actually not exported afaik) and shifting this to AbstractMCMC means that it's easier for other packages to make use of this info.

Turing.Inference.getparams is gone now, it's replaced with AbstractMCMC.getparams (but called on the state instead of the transition).

Comment on lines -20 to -24
Turing.Inference.isgibbscomponent(::SliceSampling.RandPermGibbs) = true
Turing.Inference.isgibbscomponent(::SliceSampling.HitAndRun) = true
Turing.Inference.isgibbscomponent(::SliceSampling.Slice) = true
Turing.Inference.isgibbscomponent(::SliceSampling.SliceSteppingOut) = true
Turing.Inference.isgibbscomponent(::SliceSampling.SliceDoublingOut) = true
Copy link
Member Author

Choose a reason for hiding this comment

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

isgibbscomponent now defaults to true so it no longer needs to be overridden

Comment on lines +46 to +55
abstract type AbstractStateWithTransition end
AbstractMCMC.getparams(state::AbstractStateWithTransition) = state.transition.params
AbstractMCMC.getstats(state::AbstractStateWithTransition) = state.transition.info
function AbstractMCMC.setparams!!(
model::AbstractMCMC.LogDensityModel, state::AbstractStateWithTransition, params
)
new_lp = LogDensityProblems.logdensity(model.logdensity, params)
new_transition = Transition(params, new_lp, NamedTuple())
return Accessors.@set state.transition = new_transition
end
Copy link
Member Author

Choose a reason for hiding this comment

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

Since the definitions of these functions are the same for all states in this package, I thought it would be cleaner to just define the behaviour on an abstract type. It does necessitate an extra dep on Accessors, but that's fairly lightweight.

@penelopeysm
Copy link
Member Author

penelopeysm commented Dec 14, 2025

The failing tests are to do with initial_sample here

function SliceSampling.initial_sample(rng::Random.AbstractRNG, ℓ::Turing.LogDensityFunction)
n_max_attempts = 1000
model, vi =.model, ℓ.varinfo

The fact is that when SliceSampling is used via Turing's externalsampler mechanism, that code path will never be hit, because externalsampler makes sure to provide a vector of initial params:

https://github.com/TuringLang/Turing.jl/blob/4dc7ad096f92fd571de312e7751986484ac6cb50/src/mcmc/external_sampler.jl#L151-L177

So this code path will only be hit if somebody actually manually constructs a LogDensityFunction and passes it to SliceSampling without specifying initial params. One option is therefore to delete the function and the tests. Even though this seems like a bit of a niche use case, I also don't see any justifiable reason to not support it, and deleting it seems overkill.

If we don't go down that route, we need a patch to make it work again, since DynamicPPL 0.39 removed some functionality that was needed for this. That patch is here: TuringLang/DynamicPPL.jl#1178. Thus, we need to wait for that to be merged, and I can revisit this after that.

Once that is done, I think that the Turing ext can be changed into a DynamicPPL ext, which will make life easier for everyone (more accurate compat bounds, no need to update when there are unrelated changes in Turing, etc.)

@Red-Portal
Copy link
Member

Red-Portal commented Dec 14, 2025

So this code path will only be hit if somebody actually manually constructs a LogDensityFunction and passes it to SliceSampling without specifying initial params. One option is therefore to delete the function and the tests. Even though this seems like a bit of a niche use case, I also don't see any justifiable reason to not support it, and deleting it seems overkill.

I think it is fine to remove the custom initialization. After all, the purpose of that ext is to make things work automatically with Turing, nothing more.

Once that is done, I think that the Turing ext can be changed into a DynamicPPL ext, which will make life easier for everyone (more accurate compat bounds, no need to update when there are unrelated changes in Turing, etc.)

Yes, sounds good to me. Let me handle that once this PR is merged.

@Red-Portal
Copy link
Member

Hi @penelopeysm , what are your plans for this PR?

@penelopeysm
Copy link
Member Author

Apologies, I took an actual Christmas break and this slipped off the radar. I'll tidy it up tomorrow or something.

@penelopeysm
Copy link
Member Author

I think it is fine to remove the custom initialization.

In that case, the entire extension can be removed! Essentially, all the functions that need to be overloaded are now in AbstractMCMC, so they just live in src. I've updated the PR by, well, simply removing the extension.

I think in Project.toml the extras section for test deps is not needed too, since you have a test/Project.toml, so I removed those too to avoid redundant information. It all works locally for me.

Finally, I also removed the tests that used to check the behaviour of initial_sample.

Now unfortunately CI will still fail -- that is due to a DynamicPPL bug, which I'll fix now.

@penelopeysm
Copy link
Member Author

TuringLang/DynamicPPL.jl#1202 should fix it, but I'll only be able to check tomorrow.

@penelopeysm penelopeysm marked this pull request as ready for review January 17, 2026 12:24
Copy link
Member

@Red-Portal Red-Portal left a comment

Choose a reason for hiding this comment

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

All looks great to me. Seems like we don't even need a DynamicPLL extension now, is that correct?

@penelopeysm
Copy link
Member Author

Yup, that's right :)

@Red-Portal
Copy link
Member

Red-Portal commented Jan 17, 2026

@penelopeysm Could you add a one-liner docstring to AbstractStateWithTransition so that Documenter doesn't complain? I'll merge the PR immediately after that.

@penelopeysm
Copy link
Member Author

Ah apologies I should have checked CI haha

@github-actions
Copy link
Contributor

SliceSampling.jl documentation for PR #49 is available at:
https://TuringLang.github.io/SliceSampling.jl/previews/PR49/

@Red-Portal Red-Portal merged commit ab02eb5 into main Jan 17, 2026
8 checks passed
@Red-Portal Red-Portal deleted the py/turing branch January 17, 2026 22:45
@Red-Portal
Copy link
Member

This is really nice. Thank you very much!

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