-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix type instabilities of SimplexBijector #174
Conversation
I'll review, but quick quesh first: is there a reason why you re-introduced the |
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 |
For the case when |
Sure, you can hardcode it for the default version with |
BTW also with your suggestion in TuringLang/Turing.jl#1276 (comment) |
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 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 😕 So I guess I'm in favour of adding back the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
But is this considered a breaking change? Should we bump the minor version or? (Also can you bump the version?:) )
Yes, IMO this is a breaking change. |
Fixes type instabilities discovered in TuringLang/Turing.jl#1276.