Make subset preserve varname ordering in varinfo #832
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #832 +/- ##
==========================================
- Coverage 84.60% 84.56% -0.04%
==========================================
Files 34 34
Lines 3832 3830 -2
==========================================
- Hits 3242 3239 -3
- Misses 590 591 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mhauru
changed the title
Pull Request Test Coverage Report for Build 13699666388Details
💛 - Coveralls |
|
|
| ) where {names,sym} | ||
| return if (sym in names) | ||
| # TODO(mhauru) Note that this could still generate an empty metadata object if none | ||
| # of the lenses in `vns` are in `metadata`. Not sure if that's okay. Checking for |
There was a problem hiding this comment.
I think this is fine. But maybe we could add some isempty check where we use subset?
There was a problem hiding this comment.
I think that would be preferable, but I don't know how to do it in a type stable manner. Specifically, I would like to implement it so that if we get a TypedVarInfo where for some symbol :dada the corresponding Metadata object is empty, we would drop :dada from the NamedTuple. But that means that the type of the return value depends on the value of the input (and not just the type of the input), since the keys of a NamedTuple are a part of its type.
I don't think so tbh, so this seems completely fine to change to me 👍 |
| @test [varinfo_merged[vn] for vn in vns] == [varinfo[vn] for vn in vns] | ||
| end | ||
|
|
||
| @testset "$(convert(Vector{VarName}, vns_subset)) order" for vns_subset in |
There was a problem hiding this comment.
Do you feel confident that all the methods we have for VarInfo will work correctly with "empty" NamedTuple entries in VarInfo? 👀
There was a problem hiding this comment.
As in, is there more to be tested here or do we feel confident about this converage?
There was a problem hiding this comment.
Do you feel confident that all the methods we have for VarInfo will work correctly with "empty" NamedTuple entries in VarInfo? 👀
Confident, nope. Hopeful at best. I think this PR makes the situation marginally better though, in that it is a bit harder to create VarInfos with empty Metadatas using subset now. We should do all sorts of testing around such pathological VarInfos, but I think that would be a different PR.
I was about to say that delete! probably creates similar issues, but actually, seems delete! is broken/unimplemented for TypedVarInfo anyway. Should fix that too.
There was a problem hiding this comment.
Yeah, definitively think this PR is making the situation better:) I was mainly trying to elude to whether we should add a few more test cases here / add an "empty" varinfo to be tested somewhere else.
There was a problem hiding this comment.
Opened an issue about it here: #839
Previously,
subset(vi, vns)had the variables in the return value ordered according tovnsand not according to their order invi. This PR makes the order be the same as that invi.This fixes the issue in TuringLang/Turing.jl#2300 (comment).
Since the docstring of
subsetpreviously said that the order was not guaranteed to be the same asvi, without saying what the order would be, I've marked this as a patch rather than a breaking release. I think this is debatable, so feel free to debate it.@torfjelde, was there a particular reason to not do this before, other than no one bothering to implement it?