-
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
Add support for distributions with monotonically increasing bijector
#297
Changes from 2 commits
459082a
15ec103
011e41b
5ffa2ea
03fc4b4
d37da88
94bc3b9
ce26a29
c303eaa
84bd3ec
e8504f6
bab389d
3d404f1
505b92e
9144887
064de1e
790f62c
565aaf6
1cec356
e8cb6f9
2e89ce2
003c488
1929040
98f993c
173be81
85285f0
cb24aa3
fbc6ec3
bdc547c
f87a87b
d496221
35e7d31
1d6d5a6
6bc4a17
baf15c8
826032d
8b9761f
deece0f
d287a96
08ac70a
3ebbb12
688ecb7
4577c00
c610ec9
c837965
fa60b5d
6873d96
9f2aa92
c9304af
7b67cbf
79aa3ec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -230,6 +230,23 @@ transform!(::typeof(identity), x, y) = copy!(y, x) | |||||||||
logabsdetjac(::typeof(identity), x) = zero(eltype(x)) | ||||||||||
logabsdetjac!(::typeof(identity), x, logjac) = logjac | ||||||||||
|
||||||||||
################### | ||||||||||
# Other utilities # | ||||||||||
################### | ||||||||||
""" | ||||||||||
is_monotonically_increasing(f) | ||||||||||
|
||||||||||
Returns `true` if `f` is monotonically increasing. | ||||||||||
""" | ||||||||||
is_monotonically_increasing(f) = false | ||||||||||
is_monotonically_increasing(::typeof(identity)) = true | ||||||||||
function is_monotonically_increasing(cf::ComposedFunction) | ||||||||||
return is_monotonically_increasing(cf.inner) && is_monotonically_increasing(cf.outer) | ||||||||||
end | ||||||||||
is_monotonically_increasing(::typeof(exp)) = true | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this is an interface function, would it be better to place these methods in the files where the corresponding bijectors are implemented? Also, I think we can mark this as true for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed:) |
||||||||||
is_monotonically_increasing(ef::Elementwise) = is_monotonically_increasing(ef.x) | ||||||||||
|
||||||||||
|
||||||||||
torfjelde marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
###################### | ||||||||||
# Bijectors includes # | ||||||||||
###################### | ||||||||||
|
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.
Would it be useful to add to the docstring a comment about when this is safe to use? e.g. in general, if
d
has variable parameters, then this is unsafe, because it implicitly renormalizes the distribution, but the log of the new normalization constant is never computed so is never included in the log density or gradient, though it would need to be when the parameters are variable.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.
When you say "variable parameters", do you mean that the size / length can vary?
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.
No, I mean parameters that are being sampled, optimized in the model.
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.
So you're saying
will be incorrect?
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.
Okay so I understand what you mean by this now, but I'm struggling a bit to understand why this is an issue here but not for other "typical" diffeomorphisms. The jacobian correction is exactly so we don't have to worry about normalization, no?
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.
Well another way to put it is that the function is not in fact a diffeomorphisms, because it's injective, not bijective. The original distribution is defined on some support, but
ordered(...)
has a support that is a subset of the original distribution. So we're not just applying a bijective map and worrying about the change of measure. We're additionally restricting the support, which the Jacobian cannot capture. If we had a magic function that could compute normalization factors, then we would use that to compute the normalized logpdf after restricting the support, and then Ordered would indeed be a bijection.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.
So this I can get behind for the ordered, but I don't get this for exp?
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.
Same deal. Because you're taking a density whose support is the whole real line and using a change of variables from the real line to the positive reals (not the whole real line). So you're changing the support if you use
Exp
for this distribution.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.
Nah, then I still don't get it. I'm with you that you're changing the domain, but$\exp: \mathbb{R} \to (0, \infty)$ is bijective, which are the domains we are interested in. Why does it matter that $\exp$ treated as a map from $\mathbb{R} \to \mathbb{R}$ is not bijective? o.O
Could you maybe write down exactly what you mean mathematically, or alternatively point me to a resource where I can read about this? I don't think I fully understand exactly what issue you're pointing to here, but really feel like this is something I should know so very keen on learning more.
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.
Btw just implemented sampling from the following model:
and even that fails to produce consistent results 😕
Are these two really supposed to be the same?
Here's the output of the above:
As you can see, the quantiles are different.