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

Truncated should catch truncations outside the support #843

Open
tpapp opened this issue Mar 17, 2019 · 4 comments · May be fixed by #1723
Open

Truncated should catch truncations outside the support #843

tpapp opened this issue Mar 17, 2019 · 4 comments · May be fixed by #1723

Comments

@tpapp
Copy link
Contributor

tpapp commented Mar 17, 2019

Eg

Truncated(Exponential(1.0), -3.0, -1.0)

should error, as it leads to an invalid distribution.

Technically, for a continuous distribution d with l, r = extrema(d), Truncated(d, L, R) should error unless [l,r] and [L,R] intersect.

(thanks to @mschauer for pointing this out, cf discussion in #842)

@matbesancon
Copy link
Member

@tpapp I assume this was fixed in #842 ?

@tpapp
Copy link
Contributor Author

tpapp commented May 8, 2019

No, in general (for Truncated) it wasn't, you still can do eg

julia> Truncated(Gamma(1.0, 2.0), -5.0, -3.0)
Truncated(Gamma{Float64}=1.0, θ=2.0), range=(-5.0, -3.0))

I think it should be fixed by Truncated checking things.

Can you reopen?

@matbesancon matbesancon reopened this May 8, 2019
@matbesancon
Copy link
Member

ok sorry for that, I was cleaning things up this morning

@quildtide
Copy link
Contributor

quildtide commented Feb 14, 2021

This is still an issue now; I'd like to tackle fixing this, but there's a couple layers to this, and I'm curious what the general consensus is on how this issue should be fully resolved.

Option 1: Allow Truncated Distributions with No Support

Keeping the status quo results in many weird effects like:

julia> rand(Distributions.truncated(Distributions.Beta(1, 1), 2, 3))
1.0

julia> rand(Distributions.truncated(Distributions.Beta(1, 1), -5, -1))
0.0

julia> rand(Distributions.truncated(Distributions.Binomial(1, 1), -5, -1))
0

julia> rand(Distributions.truncated(Distributions.Binomial(1, 1), 2, 3))
1

There are additional weird effects like:

julia> d = Truncated(Gamma(1.0, 2.0), -5.0, -3.0)
Truncated(Gamma{Float64}(α=1.0, θ=2.0), range=(-5.0, -3.0))

julia> maximum(d)
-3.0

julia> minimum(d)
0.0

That distribution has a higher reported minimum than a maximum, although both values are ultimately nonsense as the distribution isn't actually supported anywhere; they should theoretically be NaN.

If we continue to allow distributions with no support, we should probably error or warn on rand calls and NaN on many other function calls (e.g. maximum, minimum, pdf, cdf, quantile, etc).

Tracking down all edge cases of function calls to specific distributions might take awhile, but this can theoretically be done incrementally as new problem distributions are found. This requires a lot of potential if statements added to existing methods though.

Option 2: Error on Truncated Distributions with No Support

Upon attempting to create a truncated distribution with no support, we should error out.

This requires significantly fewer checks, and it seems to be the first instinctive solution.

However, there is a major drawback: Existing code that generates parameters for truncated distributions and occasionally generates these invalid distributions will "break".

I say "break" as they are probably already broken if anything is being done with the 0-support distributions, but it is currently a silent break that does not interrupt the program's execution. In some cases where a large amount of truncated distributions are generated, and only occasionally is one generated with 0 support, this effect might be negligible, and the code might work more or less fine for its purpose (e.g. Monte Carlo or something).

In other cases, those invalid truncated distributions might have only exist temporarily, and the exact parameters might be updated (e.g. through exotic approximations of Bayesian processes) before these 0-support distributions are used for anything.

I do not believe it would be optimal for code that currently "works" to break when we fix this, but there is a legitimate argument that this code is usually already broken (just silently).

Option 3: Warn on Truncated Distributions with No Support

I personally support this method. Distributions.jl could warn users when a distribution with no support is generated. This is better than the status quo, because existing code that uses (probably not intentionally) 0-support truncated distributions currently does not make the user aware that their code is probably already somewhat broken.

If Distributions.jl takes this route, there is still a question as to whether methods should be aware of these 0-support truncated distributions or not.

Option 3a: Ignore Truncated Distributions with No Support

We can keep the status quo, and avoid having to track down cases like the above-mentioned truncated gamma d where minimum(t) > maximum(t) according to Distributions.jl. In this case, the warning on creating a 0-support truncated distribution should inform users that function calls may return completely unreliable and inaccurate results.

This requires a minimal amount of work, but there may be some edge cases with certain rand methods that implement accept-reject algorithms that are somehow reliant on minimum and maximum resulting in infinite loops. These can however be resolved as they are found.

Option 3b: Be Aware of Truncated Distributions with No Support

This requires a similar amount of work as Option 1. However, there is some value in allowing code akin to the following to evaluate true.

maximum(truncated(Uniform(0, 1), 2, 3)) == NaN

I personally support incrementally moving in this direction (making changes whenever an inconsistency is found). Moving towards this direction might entail the creation of several new edge case tests (perhaps their own category), but I think it is worth it.

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 a pull request may close this issue.

3 participants