-
Notifications
You must be signed in to change notification settings - Fork 36
Replace Metadata.flags
with Metadata.trans
#1060
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
base: breaking
Are you sure you want to change the base?
Conversation
Benchmark Report for Commit a011dd6Computer Information
Benchmark Results
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## breaking #1060 +/- ##
============================================
+ Coverage 82.39% 82.51% +0.11%
============================================
Files 42 42
Lines 3818 3786 -32
============================================
- Hits 3146 3124 -22
+ Misses 672 662 -10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
DynamicPPL.jl documentation for PR #1060 is available at: |
CI benchmarks. Target branch:
This branch:
Roughly in line with what I saw locally. Seems worth it to me, especially if you look at the Loop univariate 1k and 10k models. |
I suggest we take this chance to rename |
Good idea, done. |
# TODO(mhauru) Eventually I would like to rename the is_transformed function to | ||
# is_unconstrained, but that's significantly breaking. | ||
""" | ||
istrans(vnv::VarNamedVector, vn::VarName) | ||
is_transformed(vnv::VarNamedVector, vn::VarName) |
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.
Are you still thinking of this? I personally prefer islinked over istransformed, but isunconstrained / isconstrained I don't like, because it doesn't accurately capture the full lstory.
For example, unlinked variables can still be unconstrained. So is_unconstrained
doesn't mean it's unconstrained, it means it's 'guaranteed' to be unconstrained. Also, I suppose linking need not necessarily unconstrain it, it depends on the link function.
But I realise this comment might be a bit out of date
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.
For this PR, I wonder if it is worth standardising. We have islinked(::VarInfo)
but istransformed(::VarInfo, ::VarName)
. Should we change one to the other?
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.
Good point. I kinda punted on the is_unconstrained
thing in VarNamedVector because it's invisible to users, but islinked
is a good point. Now would be as good a time as any to standardise.
With VarNamedVector, I went with is_unconstrained
exactly because having a non-trivial transformation does not guarantee that the variable doesn't remain constrained, and because the flag exists to guarantee unconstrainedness (of user interest) not that some transformation has been applied (not of user interest). The docstring for VarNamedVector says this:
vector of booleans indicating whether a variable has been explicitly transformed to
unconstrained Euclidean space, i.e. whether its domain is all of `ℝ^ⁿ`. If
`is_unconstrained[varname_to_index[vn]]` is true, it guarantees that the variable
`vn` is not constrained. However, the converse does not hold: if `is_unconstrained`
is false, the variable `vn` may still happen to be unconstrained, e.g. if its
original distribution is itself unconstrained (like a normal distribution).
I was quite pleased with that when I was writing that part of VarNamedVector, but then when I tried to use the same terminology in VarInfo yesterday I wasn't happy with it anymore. Unfortunately I can't now recall why I was unhappy with it... It seems fine to me when I think about it now.
islinked
(or is_linked
) feels a lot like is_transformed
: It says that some link transformation has been applied, not that it's achieved the goal of making this variable unconstrained. Although maybe I misunderstand how people use the term "link" here.
At least right now, I think is_unconstrained
is the best description of the flag, but especially if you dislike it, I would go with is_linked
, just to match the link
and unlink
function names, which I wouldn't want to change (and calling them unconstrain
and ununconstrain
or constrain
doesn't work).
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.
I have a slight preference for is_transformed
, which is more readable for people unfamiliar with generalised linear models. We could change link
/unlink
to transform
/untransform
.
See, e.g., https://www.tamaspapp.eu/TransformVariables.jl/stable/, which also adopts transform
for its API.
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.
because the flag exists to guarantee unconstrainedness (of user interest) not that some transformation has been applied (not of user interest)
If this were the case, then is_unconstrained
would be correct, but my understanding is that the flag does not reflect that, it just reflects whether a transformation has been applied. The settrans!!
function gets called when a transformation is applied, and that just sets the flag to true:
DynamicPPL.jl/src/varnamedvector.jl
Lines 335 to 343 in 3ff4149
""" | |
settrans!(vnv::VarNamedVector, val::Bool, vn::VarName) | |
Set the value for whether `vn` is guaranteed to have been transformed so that all of | |
Euclidean space is its domain. | |
""" | |
function settrans!(vnv::VarNamedVector, val::Bool, vn::VarName) | |
return vnv.is_unconstrained[vnv.varname_to_index[vn]] = val | |
end |
Did I miss a check somewhere to make sure that the transformation is indeed to unconstrained space?
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.
I see, that makes sense. I agree that from the user's perspective it's whether it's unconstrained that matters. With this in mind, I'm not very fussed with any name, as long as its accuracy or lack thereof is clearly documented. In particular, this part of the docstring:
If
is_unconstrained[varname_to_index[vn]]
is true, it guarantees that the variablevn
is not constrained.
should probably be updated, because it is not currently true (even if we would like it to be).
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.
I started writing a response to this, and as the response got longer I came to understand more and more how I don't really understand is_unconstrained
/is_transformed
and why we need it. (I'll just call it is_unconstrained here
.)
Say we have a variable @varname(a)
, stored in a VarNamedVector as a real value x
and a transformation function f
, so that the user-facing value of @varname(a)
is f(x)
.
One interpretation of is_unconstrained
could just be "is the domain of f
all of R^N?" If yes, then x
is unconstrained.
But that's not really what we mean with is_unconstrained
. What we mean is "is the domain of f
all of R^N, and is the image of f
equal to the domain of the prior distribution of @varname(a)
". So really is_unconstrained
is a statement about f
, and the relationship between f
and some particular model, which defines the prior for @varname(a)
. That's why link(vi)
is not a thing, it has to be link(vi, model)
.
More confusingly, there's this:
@model function nasty_model()
m ~ Exponential()
a ~ truncate(Normal(); lower=m)
end
Whether is_unconstrained(vi, @varname(a))
should, morally speaking, be true or false, depends on the value of m
.
I don't like having such a tie between a VarInfo
and a model when one doesn't explicitly reference the other. Nothing else in a VarNamedVector
is specific to, or refers to, a particular model. This makes me question why we even need this flag. I'm not yet sure if we do. I've been going through different places where we read the is_unconstrained
flag, and the following two I haven't fully understood yet:
- When executing with
InitContext
, whether a variable is linked affects whether we apply a transformation, derived from the prior distribution. See here. - Gibbs wants to make sure that the link-status of a variable is respected. E.g. if one component sampler requires linking and the other doesn't, and both sample the same variable, then we need to link/invlink between executing those two component samplers. See here.
I'm not yet sure how the above two play out in the context of linking status depending on the model, and especially on values of other variables like in nasty_model
. Once I hopefully, eventually, understand that, I should be able to understand whether we really need this flag, and if so, what it's name should be.
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.
I recognise that there is a larger discussion to be had about dynamic-support-models, but personally, I think getting correct behaviour for that case is a stretch goal.
My comments are solely focused on the difference between 'unconstrained' and 'transformed' for an ordinary, perfectly static, model. As far as I can see, right now, transformed does not imply unconstrained, and unconstrained does not imply transformed. The flag really keeps track of transformed. So we should either not call it unconstrained, or at least admit in the docstring that it is not necessarily true. The flexibility of DynamicPPL makes it very easy to get into a rabbit hole with all sorts of edge cases (we did the same with dynamic-sizes and particle MCMC), but I think we should get the semantics correct for the foundations first.
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.
The reason why I like islinked
is that we define (see Bijectors.jl) a "link" to be a 'special' kind of transformation: one which maps from the support of the prior distribution (which may itself be unconstrained) to unconstrained space. At least for static-support models, that immediately resolves any ambiguity, because if islinked = true
, that immediately implies unconstrained. However, if islinked = false
, it does not imply that it is constrained; unlike is_unconstrained = false
which, on its face, suggests that the variable is constrained.
-
This does not handle the dynamic-support case, of course! But nothing so far does, and I would like to say that this is the best of the options that we actually have.
-
This further implies that we want to forbid transformations that do not transform to unconstrained space. In other words,
StaticTransformation
. I am not sure who usesStaticTransformation
-- maybe worth a check? And if we don't forbid these, then the flag has to beistransformed
, because that's exactly what it represents.
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.
The flag really keeps track of transformed.
Welllll, not really. It keeps track of whether the transformation applied arises from a prior distribution associated with this variable. You can apply all sorts of transformations. For instance, any matrix-valued variable will have a flattening transformation applied to it, so that in my above example f
is a call to reshape
. Whether that warrants raising is_unconstrained
/is_transformed
to true depends on whether the prior distribution has as its domain all matrices of that shape, or some particular subset (e.g. symmetric ones).
I'm not arguing that we should call it is_unconstrained
. I'm arguing that this is hairy, and I don't quite know what we should call it. There's a risk that thinking about dynamic models makes this overly complicated, but I think there's also a chance that it forces us to understand what this flag truly is about, by rapidly proving wrong hypotheses wrong.
I also still think there's a chance we don't even need this flag, which would be the Gordian knot solution.
EDIT: Wrote this without seeing your latest message above.
islinked(vi::SimpleVarInfo) = istrans(vi) | ||
islinked(vi::SimpleVarInfo) = is_transformed(vi) |
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.
like this line is just the same function but duplicated. so it feels like to me we could just pick one and roll with it!
Now that the
"del"
flag is gone (#1058), the only flag that is ever used is"trans"
. Hence, no need to bother with having theDict{String, BitVector}
forMetadata.flags
, and can instead have a singleBitVector
forMetadata.trans
. EDIT: Renamed toMetadata.is_transformed
.You may wonder, given that
Metadata
is presumably on its way out, why bother? Two reasons:VectorVarInfo
, and there were some horrendous performance regressions there compared to usingMetadata
. Hence, we might not be about to switch over theVarNamedVector
imminently.Metadata.flags
field might actually be a significant cost compared to aBitVector
.My local benchmarking suggests that indeed, this makes a difference:
Before
After
Curious to see whether GHA benchmarks come out looking similar.