Skip to content
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

Throw error if ordered unsafe to use #241

Merged
merged 5 commits into from
Feb 2, 2023
Merged

Throw error if ordered unsafe to use #241

merged 5 commits into from
Feb 2, 2023

Conversation

sethaxen
Copy link
Member

As noted in #220 (comment), ordered(d) does the incorrect thing unless d is unconstrained. This PR adds a check that throws an error if this is not the case. Note that examples like this will now error even though they are fine to use with ordered, since they don't return a Identity bijector:

julia> bijector(Product(fill(Normal(), 5)))
Bijectors.TruncatedBijector{1, Float64, Float64}(-Inf, Inf)

julia> bijector(Product([Normal(), Cauchy()]))
Bijectors.TruncatedBijector{1, Vector{Float64}, Vector{Float64}}([-Inf, -Inf], [Inf, Inf])

Copy link
Member

@torfjelde torfjelde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Though can you bump the patch version?

Also, if you don't use Product but product_distribution (Product is deprecated), it will actually result in a DiagNormal, hence it will work in this particular case. It will still fail for other cases though.

Thanks @sethaxen !

@yebai yebai merged commit d8ebcd4 into master Feb 2, 2023
@delete-merged-branch delete-merged-branch bot deleted the orderederr branch February 2, 2023 18:19
@rikhuijzer
Copy link

Uhm for the next time, this should have been a breaking release, no? There could be clients running production depending on the old and broken implementation. They now have to adjust or have no output at all, which means this change is breaking.

@devmotion
Copy link
Member

depending on the old and broken implementation.

According to semver, bug fixes are non-breaking. The only problem I see here are the now-failing examples in the OP - but it seems they could be fixed by improved definitions of bijector for product distributions or an improved check that also supports TruncatedBijectors with infinite bounds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants