[Merged by Bors] - Small addition to link! and invlink!
#231
Conversation
|
bors try |
tryBuild failed: |
|
bors try |
|
@devmotion You okay with this? As I said, I'm down to doing some serious thinking on whether we can simplify things, but I think this is a nice addon for the current impl without any negative side-effects. |
| function link!(vi::TypedVarInfo, spl::AbstractSampler) | ||
| return link!(vi, spl, Val(getspace(spl))) | ||
| end | ||
| function link!(vi::TypedVarInfo, spl::AbstractSampler, spaceval::Val) |
There was a problem hiding this comment.
IIRC it is/was sometimes better to use
| function link!(vi::TypedVarInfo, spl::AbstractSampler, spaceval::Val) | |
| function link!(vi::TypedVarInfo, spl::AbstractSampler, ::Val{spaceval}) where spaceval |
instead. Not completely sure though, I just know that Julia does not specialize on T::Type and one has to use ::Type{T}. On the other hand, if it's just passed through to another function it shouldn't matter.
There was a problem hiding this comment.
But will this matter here? Given that this Val is just passed on, and then what you're suggesting is done in the method we're calling, which should force specialization, no?
There was a problem hiding this comment.
Haha, no I mean, is my logic correct?
I.e. will
f(x::Val) = g(x)
g(::Val{value}) = valueand
f(::Val{value}) where {value} = g(Val{value}())
g(::Val{value}) = valuealways lead to equivalent compiled code?
There was a problem hiding this comment.
Yes, AFAIK it should lead to the same compiled code.
| end | ||
| end | ||
| function link!(vi::TypedVarInfo, spl::AbstractSampler) | ||
| return link!(vi, spl, Val(getspace(spl))) |
There was a problem hiding this comment.
Sometimes type inference is improved with Val{...}(), but I think it's fine to not micro-optimize the implementation if there sre no issues.
There was a problem hiding this comment.
This is a good point though. I was just not 100% certain whether getspace was always known at compile-time (but couldn't find any counter-examples so happy to make the change if that's the intention!)
There was a problem hiding this comment.
It's fine as it is, just something to keep in mind.
|
|
||
| # Transforming only a subset of the variables | ||
| link!(vi, spl, Val((:m, ))) | ||
| @test all(x -> !istrans(vi, x), meta.s.vns) |
There was a problem hiding this comment.
BTW do you know about Base.Fix1 and Base.Fix2? Can be nice sometimes (IMO) in such cases.
There was a problem hiding this comment.
Ah yes, I am aware, though still getting into the habbit of using it 😅
But in this case I just copy-pasted from above, so didn't cross my mind.
|
bors r+ |
Makes it possible to only link/invlink a subset of the space of a sampler *for static models only* (as there's no equivalent for `UntypedVarInfo`). Is non-breaking since this is just adding an intermediate method to an existing implementation, giving increased flexbility. Probably don't want to encourage use of this, but it can be useful in cases such as the MH sampler TuringLang/Turing.jl#1582 (comment).
bors
bot
changed the title
link! and invlink!link! and invlink!
Makes it possible to only link/invlink a subset of the space of a sampler for static models only (as there's no equivalent for
UntypedVarInfo). Is non-breaking since this is just adding an intermediate method to an existing implementation, giving increased flexbility.Probably don't want to encourage use of this, but it can be useful in cases such as the MH sampler TuringLang/Turing.jl#1582 (comment).