Skip to content

implement mergeVariableStates! #1145

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from
Draft

implement mergeVariableStates! #1145

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
63867ad
implement mergeVariableStates
Affie Jun 25, 2025
bd1dabd
Apply suggestions from code review
Affie Jun 25, 2025
72a90e3
mergeVariableStates!(::AbstractDFG, ::Symbol,::Vector{<:VariableState})
Affie Jun 26, 2025
09e3417
add test for mergeVariableStates!
Affie Jun 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions src/services/DFGVariable.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -733,6 +733,27 @@ function mergeVariableState!(v::VariableCompute, vnd::VariableState)
return 1
end

function mergeVariableStates!(
dfg::AbstractDFG,
varLabel_state_pairs::Vector{<:Pair{Symbol, <:VariableState}},
)
cnt = asyncmap(varLabel_state_pairs) do (varLabel, state)
return mergeVariableState!(dfg, varLabel, state)
end
return sum(cnt)
end

function mergeVariableStates!(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have expected the default implementation signature to be

function mergeState!(<:Variable, <:VariableState)
  # actual implementation lives here
end

mergeState!(dfg::Factorgraph, lb::Symbol, state::VariableState) = mergeState!(getVariable(fg, lb), state)

mergeStates!(dfg::Factorgraph, ::Vector{<:Pair_}) = # pmap over vector mergeState!(dfg, pair[1], pair[2])

mergeVariableState!(w...;kw...) = mergeState!(w...;kw...) # alias

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mergeState! is not consistent with the rest of the API as we have getVariableState and therefore would expect mergeVariableState!. We need the Variable noun in get explicit to avoid ambiguities, to have a type stable implementation in julia, and to help with SDKs without multiple dispatch. It also makes for more readable and maintainable code to be more explicit -- when updating from getSolverData, it was a pain to distinguish between Variable and Factor. We can still do the alias, but that would likely be a DFG only function and not across SDKs. I would also not include [get/add/merge/delete]State[!] for DFGv1 beta work.

Does the comment ... ::Vector{<:Pair_}) mean you are happy with the new signature, what I wanted to know form this PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also see similar discussion here on possible future expansion of FactorState.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with ::Vector{Pair{Symbol, <:VariableState}}. There might be some dynamic dispatch if different Variable states are merged, but that is okay too in Julialand -- AND, we are fixing many of those cases separately with the HomotopyBelief* idea:

Copy link
Member

@dehann dehann Jul 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EDIT: Should add that the signature with implementation vs alias does not really bother the DFG v1 release -- this is only where the functional code actually resides. Both getState and getVariableState are still valid according to the DFG v1 spec. Similar for mergeState! vs mergeVariableState!.

Don't loose sight of

therefore would expect

You expect the longer signature, however, I expect zero duplication is better. For getVariableState(::VariableDataType) "variable" noun twice.

not consistent with the rest of the API

Again, that is what I was trying to warn about in issue 56. Obviously the overall philosophy of how to build signatures dominates everything BUT I'm not sure we are on the same page. Here is the top line description from the verbNoun wiki (which has been my mental picture all along):

verbNoun[Noun|Adjective][!](::NounType[,..., defaults=defaults; kwargs=defaults])

Shorter is better. The longer verbNounNoun(::NounType,...) is opposite to what I understood we had decided before. I thought we are building the shortest possible name in Julia's dispatch and then adding the longer signatures as aliases. For non-dispatch/non-overloading language SDKs we only have the longer signatures (and the core implementations are in Julia only anyway).

Do you have a reference/issue/comment somewhere on changing this default philosophy to longer (w/ noun duplication) rather than shorter names (as per wiki)?

it was a pain to distinguish between Variable and Factor.

I get you were struggling during refactor, but you have to look at this in hindsight (after DFG v1 has landed). You struggled because the old code was a mess. Your struggle was hopefully the last time the verbNoun refac needs to go through such as major cleanup.

Personally, I do not see a problem with using either: getState(::Variable_,...) or getState(:Factor_,...) and these would return different types. That's the key, Julia's ability to return different types. Julia implementations would live in getState (similar to old getSolver[d]ata), with aliases such as getFactorState.

mergeState! [...va...] mergeVariableState!.

mergeState! is pretty natural for me. Inconsistent with rest of API and struggled during refactor sound like transient reasons, and non-dispatch SDKs are basically just shims (glorified aliases) over GQL. Is there perhaps another reason for more verbose?

I do remember we discussed choice between getStateVariable vs getVariableState -- again, I was under the impression the decision from there is to only implement getState and then include alias getVariableState. In extreme cases maybe e as far as including a second alias getStateVariable (bad example).

It also makes for more readable and maintainable code to be more explicit

Proper use of multiple dispatch reduces maintenance. Verbose manual write out of what could have been dispatch increases maintenance load.

Less duplication is more readable for me, because the explicit "Variable" in function name tells me there are cases without variables. I think we should use multiple dispatch properly and only add the longer aliases to have consistency with other non-dispatch SDK languages.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: getVariableState was getVariableSolverData in the CRUD get, but I'm open if we can improve it more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to this, we should also look into what to do about the plural variable versions that SDK.rs can use to avoid function overloading - such as (being overly verbose for clarity):

function mergeVariablesVariablestates!(
    dfg::AbstractDFG,
    varLabel_state_pairs::Vector{<:Pair{Symbol, <:VariableState}},
)
function mergeVariableVariablestates!(
    dfg::AbstractDFG,
    variableLabel::Symbol,
    states::Vector{<:VariableState},
)

I'm not sure if rust wil be able to do the function overloading in some way if we just have (no plural on variable): mergeVariableVariablestates!/mergeVariablestates!/mergeVarstates!. (depending on the outcome above).

I'm leaning towards just mergeVar[iable]states!, but will have to see what can be used consistently across the API.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copilot's opinion:
Between Variablestate and Varstate, both are clear, but here’s a comparison based on your API style and the conventions in your code:

Varstate

Pros: Concise, easy to type, and fits well with your noun-based, single-capital convention.
Cons: Slightly less explicit, but still clear in context.
Variablestate

Pros: More explicit, immediately clear to new users what it refers to.
Cons: Longer, and may feel redundant since "variable" is already implied by context.
Recommendation:
If your API values brevity and you document your naming conventions, Varstate is the better choice.
If you want maximum explicitness for new users, Variablestate is also fine, but may be unnecessarily verbose.

Summary:
Varstate is preferred for your style—concise, clear, and consistent with your API.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also just cross referencing NavAbility/NavAbilitySDK.jl#288, where getVariableState was approved as API. We should change that also pending this outcome.

Copy link
Member Author

@Affie Affie Jul 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exists is another similar case. Variables and factors use just exists and all other nouns use hasObj. (The verb noun wiki says we are going with has, so just giving as an example)
The signature is just exists(fg, label) so we need to read the documentation that it will look for variable or factor existence where hasVaraible and hasFactor is a more self explanatory. What about [graph|agent]blobentries/metadata, etc. existence? With has... autocomplete gets you there. We can consider keeping exists as a DFG function that calls to has internally. That is not "implement the logic inside the shorthand method definition", but I don't really see the value in that as a hard rule and more implementation specific.

dfg::AbstractDFG,
variableLabel::Symbol,
states::Vector{<:VariableState},
)
cnt = asyncmap(states) do state
return mergeVariableState!(dfg, variableLabel, state)
end
return sum(cnt)
end

function copytoVariableState!(
dfg::AbstractDFG,
variableLabel::Symbol,
Expand Down
4 changes: 2 additions & 2 deletions test/testBlocks.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -792,8 +792,8 @@ function VSDTestBlock!(fg, v1)
@test mergeVariableState!(fg, :a, vnd) == 1

# Bulk copy update x0
@test updateVariableSolverData!(fg, [v1], :default) == nothing

@test DFG.mergeVariableStates!(fg, [:a=>vnd]) == 1
@test DFG.mergeVariableStates!(fg, :a, [vnd]) == 1
altVnd = vnd |> deepcopy
keepVnd = getVariableState(getVariable(fg, :a), :parametric) |> deepcopy
altVnd.infoPerCoord .= [-99.0;]
Expand Down
Loading