Gen.jl icon indicating copy to clipboard operation
Gen.jl copied to clipboard

Gen.set_submap! breaks for non-`DynamicChoiceMap` submaps

Open sritchie opened this issue 1 year ago • 2 comments

This problem comes up because there is no interface method for setting values in a choice map. For example:

julia> cm = Gen.choicemap()


julia> Gen.set_submap!(cm, :k, Gen.DynamicDSLChoiceMap(Trie{Any,Gen.ChoiceOrCallRecord}()))


julia> cm
│
└── :k


julia> Gen.set_value!(cm, (:k => :k2), "cake")
ERROR: MethodError: no method matching set_value!(::Gen.DynamicDSLChoiceMap, ::Symbol, ::String)
Closest candidates are:
  set_value!(::DynamicChoiceMap, ::Any, ::Any) at ~/.julia/packages/Gen/ME5el/src/choice_map.jl:741
  set_value!(::DynamicChoiceMap, ::Pair, ::Any) at ~/.julia/packages/Gen/ME5el/src/choice_map.jl:746
Stacktrace:

A fix now would be to put a type on set_submap! so that new_node is only allowed to be a DynamicChoiceMap, or something that supports [] setting.

sritchie avatar Nov 07 '23 14:11 sritchie

A fix now would be to put a type on set_submap! so that new_node is only allowed to be a DynamicChoiceMap

A run of the test suite with this proposed fix suggests that the Gen internals currently rely on the ability to invoke Gen.set_submap! on a DynamicDSLChoiceMap, specifically in the following line of code:

https://github.com/probcomp/Gen.jl/blob/05709187d79316464623625e370b7cf59706d2ea/src/dynamic/update.jl#L175

The problem now is that the discard, which is a DynamicChoiceMap, may contain a submap that is of type DynamicDSLChoiceMap. If the user tries to call set_value! on the discard choicemap, they will obtain the error from the original post.

fsaad avatar Nov 07 '23 16:11 fsaad

@fsaad could that line could change to use merge? Typing this quickly without staring so there could be an obvious problem but that seemed the one clear place in the API for sort-of-mutation on general ChoiceMaps.

sritchie avatar Nov 07 '23 16:11 sritchie