probcomp / Gen.jl

A general-purpose probabilistic programming system with programmable inference
https://gen.dev
Apache License 2.0
1.79k stars 160 forks source link

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

Open sritchie opened 8 months ago

sritchie commented 8 months ago

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.

fsaad commented 8 months ago

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.

sritchie commented 8 months ago

@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.