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! [...vs...] 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.

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.

Copy link
Member

@dehann dehann Jul 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

edit: technically we can just have add!, but that is not the case, so I don't think that hase been the design so far. We briefly discussed this i a call and decided to go with addVariable! for dfgv1.

I remember from this discussion that logic would go in the shorthand and alias for the longer signatures to match other languages that cannot dispatch.

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)?
Is this decision written somewhere so i can follow the reasoning.

yep, I'm asking the same question.

Personally, I do not see a problem with using either: getState(::Variable_,...) or getState(:Factor_,...) and these would return different types. [skipping your reply as vague but will continue from the phone call conversation that was better]

I previously missed your point that dispatch, although near infinite, is not always type stable. So getState(::AbstractDFG, ::Symbol) could return either FactorState or VariableState from the same signature -- hence require slow dynamic dispatch. This for me is a good reason for a more verbose signature. HOWEVER, the shorter signature for getState(::AbstractVariable / ::AbstractFactor) should still produce type stable code and therefore good. Only the highest level API calls may be a problem for short signatures, but lower down should be good.

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

From phone call, the answer is likely DFG v1.x where shorter signatures are added or just logic moved around, but no breaking changes. Sounds like most important is just getting the verb nouns consistent all over as necessary step -- i.e. focus on that for now.

mergeVariablesVariablestates!

This is exactly why I favor the short names with dispatch and don't want to handicap Julia dispatch design for the sake of the other languages. So I would add the verbose signatures as aliases over short dispatches like just add! or merge! -- essence of good design once we get there (will take time to get that clean a design though).

For this case I would just go with mergeVariableStates!:

function mergeVariableStates!(
    ::AbstractDFG,
    ::Symbol,
    ::Vector{<:Pair{Symbol, <:VariableState}},
)

getVarstate and getFacstate

I don't like these short names. In that case better to go with longer getVariableState or getFactorState.

copilot's opinion:

Nope, I disagree with copilot but thanks for checking and copying that over.

Copy link
Member

@dehann dehann Jul 9, 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)

Sounds like the verbNoun wiki/dictionary is wrong. While has and exist may both be there, the definitions should make it clear which one is the best.

We can consider keeping exists as a DFG function that calls to has internally.

depends on what the best definitions for exist and has are. If the definitions are bad then I suggest you fix/change/update the definitions first and then update the code second. Don't just follow the dictionary if it doesn't make sense, change the dictionary.

The signature is just exists(fg, label) so we need to read the documentation that it will look for variable or factor. What about [graph|agent]blobentries/metadata, etc. existence? With has... autocomplete gets you there.

Again, I would expect to just find exists and tab complete for 100's of dispatches with a single clear docustring, and a few related examples (but limited to stable types per signature). Pretty sure that is what core Julia is doing already. I'd be okay to compromise for now with always searching both variable and factors and rather preserve the short signature exists(fg, lbl) , while avoiding dynamic types..

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.

on this I would again just say that when the design is perfect, then only a few high level dispatches would cover everything, but that quality design is going to take time and instead solve under DFG v1.x guidelines -- i.e. make the decision that avoids breaking changes during DFG v1.x series releases and that allows the signatures to become shorter and more Julia dispatchy as the minor releases roll.

Copy link
Member Author

@Affie Affie Jul 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

mergeVariablesVariablestates!

This is exactly why I favor the short names with dispatch and don't want to handicap Julia dispatch design for the sake of the other languages. So I would add the verbose signatures as aliases over short dispatches like just add! or merge! -- essence of good design once we get there (will take time to get that clean a design though).
I don't like these short names. In that case better to go with longer getVariableState or getFactorState.
and other replies...

I think you are still missing the issue here. The current nouns are VariableState and FactorState not just State

  • that breaks the capital rule as with BlobEntry vs Blobentry -- but this as a rule might be a mistake
  • That causes the shorthand to be getVariableState and the long version to be getVariableVariableState

So, let me ask directly in another way, how would you like to distinguish between VariableState and FactorState structs?
Keep in mind that that will be the Noun used in CRUD.

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