Replace Metadata.flags with Metadata.trans
#1060
Conversation
Benchmark Report for Commit a011dd6Computer InformationBenchmark 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.
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.
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.
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).
| islinked(vi::SimpleVarInfo) = istrans(vi) | ||
| islinked(vi::SimpleVarInfo) = is_transformed(vi) |
There was a problem hiding this comment.
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 singleBitVectorforMetadata.trans. EDIT: Renamed toMetadata.is_transformed.You may wonder, given that
Metadatais 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 theVarNamedVectorimminently.Metadata.flagsfield 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.