Conversation
Anonymous variables were all registered under the constant key `VariableNameAnonymous`, and `Context`'s `setindex!` is insert-or-update, so a context creating more than one anonymous variable kept only the last in `individual_variables`. The graph itself was correct, but anything enumerating the context registry (notably `VarDict`) silently missed the earlier ones. Adds `add_anonymous_node!`, mirroring `add_constant_node!`, which registers under `to_symbol(VariableNameAnonymous, label.global_counter)`. The node property `name` is unchanged, so `is_anonymous` and `as_variable(VariableNameAnonymous)` are unaffected. Closes #311. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #325 +/- ##
==========================================
+ Coverage 90.72% 90.73% +0.01%
==========================================
Files 16 16
Lines 2263 2267 +4
==========================================
+ Hits 2053 2057 +4
Misses 210 210 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #311.
Problem
Every anonymous variable was registered in its
Contextunder the constant keyVariableNameAnonymous = :anonymous_var_graphppl.Context'ssetindex!routes that toset!(c.individual_variables, key, val), which is insert-or-update, so when a context creates two ormore anonymous variables (e.g. the
x + 1andy + 1sub-terms ofz ~ Normal(x + 1, y + 1)) eachone silently overwrote the previous. The registry kept only the last.
The factor graph itself was always correct — each anonymous variable is a distinct vertex with the
right edges — so this was latent. But anything enumerating
individual_variables(context)(notablyVarDict/getvardict) silently missed the earlier ones.Change
Added
add_anonymous_node!, mirroring the existingadd_constant_node!, which registers underto_symbol(VariableNameAnonymous, label.global_counter)and returns the label explicitly. The fourcall sites in
materialize_anonymous_variable!now use it.Constants already work exactly this way (
constvar_10,constvar_13, ...), which is what made theanonymous keying look unintended in the first place.
Compatibility
The node property
nameis unchanged, so nothing that identifies anonymous variables breaks:is_anonymous(graph_engine.jl:737) readsVariableNodeProperties.nameas_variable(VariableNameAnonymous)filtering comparesgetname(getproperties(...)), not the keyBoth are property-based, so all 14 existing assertions in
test/graph_construction_tests.jlpassuntouched. Nothing in
src/orext/looks up the literal:anonymous_var_graphpplkey.The one visible change:
VarDict/getvardictandContext'sshownow list one entry peranonymous variable instead of one in total, and
haskey(context, :anonymous_var_graphppl)is nowfalse. This is the intended fix — those entries were being lost — and it matches howconstvar_Nalready appears there. Worth knowing for downstream consumers that iterate
VarDict.Tests
New test item in
test/graph_construction_tests.jl: a submodel creating two anonymous variables in onecontext, asserting the number of distinct anonymous keys in
individual_variablesequals the number ofanonymous vertices in the graph, and that
is_anonymousstill identifies both.Verified it fails on
main(2 failures) and passes here. Full suite on Julia 1.12:259/259 test items, all passing.
🤖 Generated with Claude Code