Improve State.belief semantics and add explicit topology; mark getSolverParams as deprecated#1227
Improve State.belief semantics and add explicit topology; mark getSolverParams as deprecated#1227
Conversation
…Params as deprecated
There was a problem hiding this comment.
Pull request overview
This PR clarifies belief/state semantics by renaming the serialized belief container and introducing an explicit “topology” trait for homotopy beliefs, while also deprecating getSolverParams as part of the longer-term plan to remove SolverParams from the DFG object.
Changes:
- Rename
StoredBelief→StoredHomotopyBeliefand addAbstractHomotopyTopology(Roots/Leaves/Tree topologies + explicit parent arrays). - Rename/reshape belief fields (
covariances→shapes,bandwidth→bandwidths::Vector) and update serialization, printing, equality/compare, and tests accordingly. - Deprecate
getSolverParams(dfg)with a runtime depwarn; adjust a couple of module exports.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/testSerializingVariables.jl | Updates JSON round-trip tests to new belief type/topology and shapes field. |
| src/services/print.jl | Switches printed KDE bandwidth output to the new bandwidths representation. |
| src/services/compare.jl | Updates State comparison to use refBandwidths. |
| src/services/AbstractDFG.jl | Implements deprecation warning for getSolverParams. |
| src/entities/equality.jl | Updates generated compare union to the renamed belief type. |
| src/entities/State.jl | Introduces topology trait/types; renames and extends stored belief schema; updates state accessors. |
| src/Serialization/StateSerialization.jl | Migrates old-state unpacking to construct StoredHomotopyBelief. |
| src/DistributedFactorGraphs.jl | Changes @format_str / @defStateType visibility to exported. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| weights = Float64[], | ||
| points = P[], | ||
| bandwidth = nothing, | ||
| bandwidths = Matrix{Float64}[], |
There was a problem hiding this comment.
StructUtils.fielddefaults(::Type{StoredHomotopyBelief{T,P}}) sets topologykind = LeavesOnlyTopology() but leaves bandwidths empty. That combination makes the default object internally inconsistent (Leaves-only beliefs typically require at least one bandwidth, and refBandwidth currently assumes index 1 exists). Consider initializing bandwidths to [zeros(getDimension(T), getDimension(T))] when the default topology is LeavesOnlyTopology, or switching the default topology to one that doesn't require bandwidths.
| bandwidths = Matrix{Float64}[], | |
| bandwidths = [zeros(getDimension(T()), getDimension(T()))], |
| println(ioc, round.(bandwidth; digits = 4)) | ||
| bws = vnd.belief.bandwidths | ||
| if !isempty(bws) | ||
| println(ioc, round.(bws[1]; digits = 4)) |
There was a problem hiding this comment.
When bandwidths is empty, this block prints the "kde bandwidths" label but never prints a newline, so subsequent output (e.g., the "VNDs" line) will be concatenated onto the same line. Consider always emitting a newline (e.g., print []/<none> when empty) to keep the short print format stable/readable.
| println(ioc, round.(bws[1]; digits = 4)) | |
| println(ioc, round.(bws[1]; digits = 4)) | |
| else | |
| println(ioc, "[]") |
getSolverParamsas deprecated towords ShouldSolverParamsremain in DFG and be serialized? #1205 (SolverParams is too ingrained to remove one shot)