implement mergeStates!

[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).
Report is 1 commits behind head on develop.

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

[dehann]

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.

[Affie]

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.

[Affie]

From triage call:
@dehann suggested Recipe as an alternate name for FactorState. In this case VariableState will just become State. Therefore, crud will be, for example, getState and getRecipe.
Factors only have one Recipe and not keyed by a label.

Reminder, this is the factor data that will become Recipe:

mutable struct FactorState
    eliminated::Bool = false
    potentialused::Bool = false
    multihypo::Vector{Float64} = Float64[] # TODO re-evaluate after refactoring w #477
    certainhypo::Vector{Int} = Int[]
    nullhypo::Float64 = 0.0
    solveInProgress::Int = 0 #TODO maybe deprecated or move to operational memory, also why Int?
    inflation::Float64 = 0.0
end

[Affie]

That sounds good to me.
State is clear enough on its own especially for someone coming from state estimation. The factor - Recipe is more advanced and used internally, Observation is the noun that wil used more externally.

[Affie]

FactorState -> Recipe looks ok to change, it doesn't feel perfect yet. It will not be exported; therefore, we don't need to sort it out just yet. We can finalize the structure with the final serialization structure.

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

[dehann]

see topic by topic responses

[Affie]

I rebased again and changed the base to match new naming from #1163. It is now just mergeStates!.

Resolved by changing the VariableState verb to just State.