implement mergeVariableStates!

[Affie]

I went back and forth on mergeVariableStates! and comparing it with updateVariableNodeData! and settled on following the verb by still providing a container. The same concept can be followed for all satellite levels (such as Blobentry)

@dehann would you mind giving your inputs.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.61%. Comparing base (8ae0b9b) to head (09e3417).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1145      +/-   ##
===========================================
+ Coverage    72.52%   72.61%   +0.09%     
===========================================
  Files           29       29              
  Lines         2428     2436       +8     
===========================================
+ Hits          1761     1769       +8     
  Misses         667      667              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[dehann]

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

[Affie]

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?

[Affie]

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

[dehann]

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:

• Replace PPE/MKD with new noun (Homotopy?) #1133

[dehann]

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.

[Affie]

Just also referencing the original issue here: #1121

[Affie]

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

[Affie]

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.

[Affie]

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.

[Affie]

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

[Affie]

Note, branch was rebased to resolve conflics.

[Affie]

Just noting differentiating between the accessor-like get and the SDK-CRUD get:

• CRUD signatures that has a AbstractDFG, AbstractBlobstore, etc. as first argument should be implemented across the SDKs and save to use.