Closed devmotion closed 3 years ago
I'll review, but quick quesh first: is there a reason why you re-introduced the Val
? I don't think that's necessary here; at least when I experimented, it wasn't.
I could not obtain type stability without it. IMO this seems reasonable since it is not possible to infer the type, or rather the type parameter proj
, of SimplexBijector
from a boolean value proj
.
For the case when proj
is a runtime parameter you mean, or nah? It is inferrable if you do SimplexBijector{true}()
.
Sure, you can hardcode it for the default version with true
- but the three argument version, with true
or false
, is still type unstable.
BTW also with your suggestion in https://github.com/TuringLang/Turing.jl/issues/1276#issuecomment-813074909 invlink(d, x)
is still type unstable. It seems you accidentally checked link(d, x)
instead?
Haha, yeah you're right! Sorry about that. I think I was just cycling through my command history to get the function call and seems like I was a bit too quick.
But regarding the 3-argument version. The reason why we replaced Val
with proj
was because when you use link
and invlink
in a different scope than Main
, you'll have to hardcode proj
to get type-stability no matter if we're using Val
or not. And if you hardcode, even when not using Val
, you should get type stability due to constant propogation.
Thouhgh in the light of the stuff we saw in the DPPL PR, I'm not so certain about that anymore:) And it seems like even something as simple as
f(d, x) = Bijectors.invlink(d, x, true)
fails :confused:
So I guess I'm in favour of adding back the Val
.
Yes, IMO this is a breaking change.
Fixes type instabilities discovered in https://github.com/TuringLang/Turing.jl/issues/1276.