Move optional features to extensions

TuringLang / AdvancedHMC.jl

Robust, modular and efficient implementation of advanced Hamiltonian Monte Carlo algorithms

https://turinglang.org/AdvancedHMC.jl/

MIT License

237 stars 41 forks source link

Move optional features to extensions #320

Closed devmotion closed 1 year ago

devmotion commented 1 year ago

This PR is https://github.com/TuringLang/AdvancedMH.jl/pull/81 for AdvancedHMC.

devmotion commented 1 year ago

I noticed that the error hint won't work as desired since DiffEqIntegrator will hit step(::AbstractLeapFrog, ...) in integrator.jl, so no error will be thrown. One could e.g. change the type structure to ensure that the default step is not hit by DiffEqIntegrator + keep the error hint or we could e.g. check in the default step function if the first argument is a DiffEqIntegrator, and in this case throw a more informative error. It feels a bit "nicer" to not add a special case to the default step but rather make sure it's never hit, but it seems that would also require more changes.

devmotion commented 1 year ago

Test failures are due to a hiccup and some deprecations in the SciML ecosystem. Guess they'll be fixed within the next hours/day.

devmotion commented 1 year ago

A few final comments:

I also bumped the Julia compat to 1.6 and added tests for it. The latest release are only compatible with Julia >= 1.6 anyway since, e.g., LogDensityProblems 2 only supports Julia >= 1.6.
I solved the error hint issue (comment above) by defining step in the package only for Union{Leapfrog{T},JitteredLeapfrog{FT,T},TemperedLeapfrog{FT,T}} instead of AbstractLeapfrog.

devmotion commented 1 year ago

Are you fine with these changes @torfjelde? Or would you prefer something else?

torfjelde commented 1 year ago

Are you fine with these changes @torfjelde? Or would you prefer something else?

I'm happy with it, but I would like @xukai92 to chime in as he might have some code that depends on this/know of cases where this would be okay.

Otherwise, I'm very happy with the changes:)