search

TuringLang / AdvancedHMC.jl

Robust, modular and efficient implementation of advanced Hamiltonian Monte Carlo algorithms
https://turinglang.org/AdvancedHMC.jl/
MIT License
228 stars 39 forks source link

DenseMetric and Component arrays (solve #344) #345

Open erathorn opened 1 year ago

erathorn commented 1 year ago

This PR attempts to solve #344

I went for the solution to preallocate the result in ∂H∂r such that the type of the input r matches the type of the output. I added tests that not only check the correct numerical output but also check the type.

Additionally, I found a typo in the inner constructor of PhasePoint, where it originally was length(ℓπ.gradient) == length(ℓπ.gradient).

erathorn commented 1 year ago

I'm wondering if we should just make compat with ComponentArrays.jl an extension instead of complicating the existing code, and then we can just overload whatever we need there. Thoughts?

I went for this "complication" because of the comment next to safe_rsimilar and the phasepoint function taking different types. Which I understood as "workarounds" without explicit dependence.

erathorn commented 1 year ago

The tests fail at this line: https://github.com/TuringLang/AdvancedHMC.jl/blob/eb9b2e0d60ef3dd85768d6e6a9f19de15b8f7130/test/metric.jl#L13C1-L13C34

The problem seems to be, that the implementations of rand do not have the correct signature.

https://github.com/TuringLang/AdvancedHMC.jl/blob/eb9b2e0d60ef3dd85768d6e6a9f19de15b8f7130/src/metric.jl#L128C1-L136C38

https://github.com/TuringLang/AdvancedHMC.jl/blob/eb9b2e0d60ef3dd85768d6e6a9f19de15b8f7130/src/utilities.jl#L5

https://github.com/TuringLang/AdvancedHMC.jl/blob/eb9b2e0d60ef3dd85768d6e6a9f19de15b8f7130/src/utilities.jl#L9C1-L12C4

However, I have not touched any of this at all. :thinking:

torfjelde commented 1 year ago

The tests fail at this line:

This is indeed strange given that the CI on master is working just fine :confused: