search

TuringLang / Bijectors.jl

Implementation of normalising flows and constrained random variable transformations
https://turinglang.org/Bijectors.jl/
MIT License
200 stars 33 forks source link

Fix bug in Stacked #184

Closed torfjelde closed 3 years ago

torfjelde commented 3 years ago

Before #177 we had very opinionated constructors that would ensure that if you were constructing a Stacked with a Tuple as the container for the bijectors, you'd also get a Tuple container for the ranges. After relaxing the requirements/assumptions made, it's now possible (but of course not encournaged) to have a combination of Array and Tuple, e.g. Stacked((Exp(), ), [1:1, ]). This will then dispatch to the impl that unrolls the map even though it shouldn't because we only specifiy the type of the first parameter in the current impl.

This PR fixes this bug by making it so we only dispatch to the @generated impl if both type-parameters are Tuple, and falls back to the non-generated impl if this is not the case.

torfjelde commented 3 years ago

Could someone maybe have a look at this @devmotion @yebai @mohamed82008 ? It's a very small PR but it's holding up tests for PRs in Turing :confused:

devmotion commented 3 years ago

Stacked((Exp(), ), [1:1, ])

Shouldn't we handle this in a type-stable way as well? Both ranges and bijectors are concretely typed and inference shouldn't be a problem. And isn't usually the array of ranges concretely typed?

torfjelde commented 3 years ago

Shouldn't we handle this in a type-stable way as well? Both ranges and bijectors are concretely typed and inference shouldn't be a problem. And isn't usually the array of ranges concretely typed?

Sorry, I was just using terminology wrong. This will be type-stable, but it just won't use @generated, i.e. be unrolled.

devmotion commented 3 years ago

Ah, I see :+1: