Merge pull request #390 from JuliaRobotics/maint/20Q2/varfacCRUD

CGDFG and GraphsDFG fix for #375 and `factor.data` usage

Author: Affie

src/CloudGraphsDFG/services/CloudGraphsDFG.jl

@@ -221,7 +221,7 @@ function getVariable(dfg::CloudGraphsDFG, label::Union{Symbol, String})::DFGVari
     if typeof(label) == String
         label = Symbol(label)
     end
-    nodeId = _tryGetNeoNodeIdFromNodeLabel(dfg.neo4jInstance, dfg.userId, dfg.robotId, dfg.sessionId, label)
+    nodeId = _tryGetNeoNodeIdFromNodeLabel(dfg.neo4jInstance, dfg.userId, dfg.robotId, dfg.sessionId, label, nodeType="VARIABLE")
     if nodeId == nothing
         error("Unable to retrieve the ID for variable '$label'. Please check your connection to the database and that the variable exists.")
     end
@@ -240,7 +240,7 @@ function getFactor(dfg::CloudGraphsDFG, label::Union{Symbol, String})::DFGFactor
     if typeof(label) == String
         label = Symbol(label)
     end
-    nodeId = _tryGetNeoNodeIdFromNodeLabel(dfg.neo4jInstance, dfg.userId, dfg.robotId, dfg.sessionId, label)
+    nodeId = _tryGetNeoNodeIdFromNodeLabel(dfg.neo4jInstance, dfg.userId, dfg.robotId, dfg.sessionId, label, nodeType="FACTOR")
     if nodeId == nothing
         error("Unable to retrieve the ID for factor '$label'. Please check your connection to the database and that the factor exists.")
     end

src/CloudGraphsDFG/services/CommonFunctions.jl

@@ -160,10 +160,15 @@ end
 $(SIGNATURES)
 Try get a Neo4j node ID from a node label.
 """
-function _tryGetNeoNodeIdFromNodeLabel(neo4jInstance::Neo4jInstance, userId::String, robotId::String, sessionId::String, nodeLabel::Symbol)::Union{Nothing, Int}
+function _tryGetNeoNodeIdFromNodeLabel(neo4jInstance::Neo4jInstance, userId::String, robotId::String, sessionId::String, nodeLabel::Symbol; nodeType::Union{Nothing, String}=nothing)::Union{Nothing, Int}
     @debug "Looking up symbolic node ID where n.label = '$nodeLabel'..."
-    nodes = _getNeoNodesFromCyphonQuery(neo4jInstance, "(node:$userId:$robotId:$sessionId) where exists(node.label) and node.label = \"$(string(nodeLabel))\"")
-    if(length(nodes) != 1)
+    if nodeType == nothing
+        nodes = _getNeoNodesFromCyphonQuery(neo4jInstance, "(node:$userId:$robotId:$sessionId) where exists(node.label) and node.label = \"$(string(nodeLabel))\"")
+    else
+        nodes = _getNeoNodesFromCyphonQuery(neo4jInstance, "(node:$nodeType:$userId:$robotId:$sessionId) where exists(node.label) and node.label = \"$(string(nodeLabel))\"")
+    end
+
+    if length(nodes) != 1
         return nothing
     end
     nodeId = nodes[1].id

src/Deprecated.jl

@@ -38,12 +38,7 @@ Base.getproperty(x::DFGFactor,f::Symbol) = begin
   elseif f == :_internalId
       getfield(x,:_dfgNodeParams)._internalId
   elseif f == :data
-
-      if !(@isdefined getFactorDataWarnOnce)
-          @warn "get: data field is deprecated, use getSolverData. Further warnings are suppressed"
-          global getFactorDataWarnOnce = true
-      end
-
+      Base.depwarn("DFGFactor get: data field is deprecated, use getSolverData", :getproperty)
       getfield(x, :solverData)
   else
       getfield(x,f)
@@ -57,12 +52,7 @@ Base.setproperty!(x::DFGFactor,f::Symbol, val) = begin
     elseif f == :_internalId
         getfield(x,:_dfgNodeParams)._internalId = val
     elseif f == :data
-
-        if !(@isdefined setFactorDataWarnOnce)
-            @warn "set: data field is deprecated, use ...TODO? Further warnings are suppressed"
-            global setFactorDataWarnOnce = true
-        end
-
+        Base.depwarn("DFGFactor set: data field is deprecated, use getSolverData", :getproperty)
         setfield!(x,:solverData, val)
     else
         setfield!(x,f,val)

src/GraphsDFG/services/GraphsDFG.jl

@@ -121,7 +121,9 @@ function getVariable(dfg::GraphsDFG, label::Union{Symbol, String})::DFGVariable
     if !haskey(dfg.labelDict, label)
         error("Variable label '$(label)' does not exist in the factor graph")
     end
-    return dfg.g.vertices[dfg.labelDict[label]].dfgNode
+    node = dfg.g.vertices[dfg.labelDict[label]].dfgNode
+    !isa(node, AbstractDFGVariable) && error("Node with label '$(label)' is not a variable")
+    return node
 end
 
 function getFactor(dfg::GraphsDFG, factorId::Int64)::DFGFactor
@@ -140,7 +142,9 @@ function getFactor(dfg::GraphsDFG, label::Union{Symbol, String})::DFGFactor
     if !haskey(dfg.labelDict, label)
         error("Factor label '$(label)' does not exist in the factor graph")
     end
-    return dfg.g.vertices[dfg.labelDict[label]].dfgNode
+    node = dfg.g.vertices[dfg.labelDict[label]].dfgNode
+    !isa(node, AbstractDFGFactor) && error("Node with label '$(label)' is not a factor")
+    return node
 end
 
 function updateVariable!(dfg::GraphsDFG, variable::DFGVariable)::DFGVariable

src/services/Serialization.jl

@@ -98,10 +98,10 @@ function packFactor(dfg::G, f::DFGFactor)::Dict{String, Any} where G <: Abstract
     props["timestamp"] = string(f.timestamp)
     props["tags"] = JSON2.write(f.tags)
     # Pack the node data
-    fnctype = f.data.fnc.usrfnc!
+    fnctype = getSolverData(f).fnc.usrfnc!
     try
         packtype = getfield(_getmodule(fnctype), Symbol("Packed$(_getname(fnctype))"))
-        packed = convert(PackedFunctionNodeData{packtype}, f.data)
+        packed = convert(PackedFunctionNodeData{packtype}, getSolverData(f))
         props["data"] = JSON2.write(packed)
     catch ex
         io = IOBuffer()
@@ -149,7 +149,8 @@ function unpackFactor(dfg::G, packedProps::Dict{String, Any})::DFGFactor where G
     factor = DFGFactor{typeof(fullFactor.fnc), Symbol}(Symbol(label), 0, timestamp)
 
     union!(factor.tags, tags)
-    factor.data = fullFactor #TODO setSolverData!(factor, fullFactor)
+    # factor.data = fullFactor #TODO 
+    setSolverData!(factor, fullFactor)
     factor._variableOrderSymbols = _variableOrderSymbols
     setSolvable!(factor, solvable)
 

test/testBlocks.jl

@@ -24,6 +24,9 @@ struct TestFunctorSingleton <: FunctorSingleton end
 struct TestFunctorPairwise <: FunctorPairwise end
 struct TestFunctorPairwiseMinimize <: FunctorPairwiseMinimize end
 
+struct PackedTestFunctorInferenceType1 end
+Base.convert(::Type{GenericFunctionNodeData{PackedTestFunctorInferenceType1,AbstractString}}, d) = d
+
 struct TestCCW{T} <: ConvolutionObject where {T<:FunctorInferenceType}
     usrfnc!::T
 end
@@ -338,7 +341,7 @@ function  VariablesandFactorsCRUD_SET!(fg, v1, v2, v3, f0, f1, f2)
     #deletions
     @test getVariable(fg, :c) === deleteVariable!(fg, v3)
     @test_throws ErrorException deleteVariable!(fg, v3)
-    @test setdiff(ls(fg),[:a,:b]) == []
+    @test issetequal(ls(fg),[:a,:b])
     @test getFactor(fg, :bcf1) === deleteFactor!(fg, f2)
     @test_throws ErrorException deleteFactor!(fg, f2)
     @test lsf(fg) == [:abf1]
@@ -356,14 +359,14 @@ function  VariablesandFactorsCRUD_SET!(fg, v1, v2, v3, f0, f1, f2)
         @test_logs (:warn, r"supported for type DFGVariable") getVariable(fg, :a, :missingfoo)
     end
 
-    @test getFactor(fg, :abf1) === f1
+    @test getFactor(fg, :abf1) == f1
 
     @test_throws ErrorException getVariable(fg, :c)
     @test_throws ErrorException getFactor(fg, :bcf1)
 
     #test issue #375
-    @test_skip @test_throws ErrorException getVariable(fg, :abf1)
-    @test_skip @test_throws ErrorException getFactor(fg, :a)
+    @test_throws ErrorException getVariable(fg, :abf1)
+    @test_throws ErrorException getFactor(fg, :a)
 
     # Existence
     @test exists(fg, :a)