Fix rand and randn type piracy
#387
Conversation
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
@devmotion Could I get you to fix the conflicts? Then I can take a look again. |
|
Done, I fixed the merge conflict. |
| Base.rand(rng::AbstractRNG, metric::AbstractMetric, kinetic, θ) = | ||
| _rand(rng, metric, kinetic) # this disambiguity is required by Random.rand | ||
| rand_momentum(rng, metric, kinetic) # this disambiguity is required by Random.rand | ||
| Base.rand(rng::AbstractVector{<:AbstractRNG}, metric::AbstractMetric, kinetic, θ) = | ||
| _rand(rng, metric, kinetic) | ||
| rand_momentum(rng, metric, kinetic) | ||
| Base.rand(metric::AbstractMetric, kinetic, θ) = rand(Random.default_rng(), metric, kinetic) |
There was a problem hiding this comment.
Additionally, I renamed the existing internal _rand function to rand_momentum to make it clearer that it's just about sampling a momentum
I'm always hugely in favour of disambiguating names so very glad to see this.
There seem to be a few of these definitions
Base.rand(args...) = rand_momentum(fewer_args...)Would it make sense to rename those Base.rand to rand_momentum as well? Happy to leave it to another time too, just curious if there's a reason you didn't do that.
There was a problem hiding this comment.
I was about to remove them but then decided to keep them in this PR because it could (should?) be considered part of the public API whereas _rand etc are surely internal.
There was a problem hiding this comment.
Hmm, I thought yes that makes sense, then looking at it more closely I realised the research subdirectory is just a bunch of scripts that use AdvancedHMC rather than part of the module itself. So I guess the name doesn't really matter so much. Feel free to hit merge if you're happy
I just came across a seemingly severe case of type piracy in AdvancedHMC. As a first step I just removed it to see what parts are currently making use of it.
Edit: I removed the definition of
Base.rand(replaced with branches in the two function where it was used) and added an internal_randnto fix the type piracy ofBase.randn(falls back toBase.randnfor single RNGs and keeps the current behaviour for vectors of RNGs). Additionally, I renamed the existing internal_randfunction torand_momentumto make it clearer that it's just about sampling a momentum and not similar to_randn.