Merge pull request #371 from JuliaRobotics/feature/20Q1/orphans296

Remove orphaned factors on deleteVariable!

Author: Affie

src/CloudGraphsDFG/services/CloudGraphsDFG.jl

@@ -304,22 +304,27 @@ function updateFactor!(dfg::CloudGraphsDFG, factor::DFGFactor)::DFGFactor
     return factor
 end
 
-function deleteVariable!(dfg::CloudGraphsDFG, label::Symbol)::DFGVariable
+function deleteVariable!(dfg::CloudGraphsDFG, label::Symbol)#::Tuple{AbstractDFGVariable, Vector{<:AbstractDFGFactor}}
     variable = getVariable(dfg, label)
     if variable == nothing
         error("Unable to retrieve the ID for variable '$label'. Please check your connection to the database and that the variable exists.")
     end
 
+    deleteNeighbors = true # reserved, orphaned factors are not supported at this time
+    if deleteNeighbors
+        neigfacs = map(l->deleteFactor!(dfg, l), getNeighbors(dfg, label))
+    end
+
     # Perform detach+deletion
     _getNeoNodesFromCyphonQuery(dfg.neo4jInstance, "(node:$label:$(join(_getLabelsForType(dfg, DFGVariable),':'))) detach delete node ")
 
     # Clearing history
     dfg.addHistory = symdiff(dfg.addHistory, [label])
-    return variable
+    return variable, neigfacs
 end
 
 #Alias
-deleteVariable!(dfg::CloudGraphsDFG, variable::DFGVariable)::DFGVariable = deleteVariable!(dfg, variable.label)
+deleteVariable!(dfg::CloudGraphsDFG, variable::DFGVariable) = deleteVariable!(dfg, variable.label)
 
 function deleteFactor!(dfg::CloudGraphsDFG, label::Symbol)::DFGFactor
     factor = getFactor(dfg, label)
@@ -397,7 +402,7 @@ function isConnected(dfg::CloudGraphsDFG)::Bool
 end
 
 function getNeighbors(dfg::CloudGraphsDFG, node::T; solvable::Int=0)::Vector{Symbol}  where T <: DFGNode
-    query = "(n:$(dfg.userId):$(dfg.robotId):$(dfg.sessionId):$(node.label))--(node) where (node:VARIABLE or node:FACTOR) and node.solvable >= $solvable"
+    query = "(n:$(dfg.userId):$(dfg.robotId):$(dfg.sessionId):$(node.label))-[r:FACTORGRAPH]-(node) where (node:VARIABLE or node:FACTOR) and node.solvable >= $solvable"
     @debug "[Query] $query"
     neighbors = _getLabelsFromCyphonQuery(dfg.neo4jInstance, query)
     # If factor, need to do variable ordering TODO, Do we? does it matter if we always use _variableOrderSymbols in calculations?
@@ -408,7 +413,7 @@ function getNeighbors(dfg::CloudGraphsDFG, node::T; solvable::Int=0)::Vector{Sym
 end
 
 function getNeighbors(dfg::CloudGraphsDFG, label::Symbol; solvable::Int=0)::Vector{Symbol}
-    query = "(n:$(dfg.userId):$(dfg.robotId):$(dfg.sessionId):$(label))--(node) where (node:VARIABLE or node:FACTOR) and node.solvable >= $solvable"
+    query = "(n:$(dfg.userId):$(dfg.robotId):$(dfg.sessionId):$(label))-[r:FACTORGRAPH]-(node) where (node:VARIABLE or node:FACTOR) and node.solvable >= $solvable"
     @debug "[Query] $query"
     neighbors = _getLabelsFromCyphonQuery(dfg.neo4jInstance, query)
     # If factor, need to do variable ordering TODO, Do we? does it matter if we always use _variableOrderSymbols in calculations?

src/GraphsDFG/services/GraphsDFG.jl

@@ -165,14 +165,21 @@ function updateFactor!(dfg::GraphsDFG, factor::DFGFactor)::DFGFactor
     return factor
 end
 
-function deleteVariable!(dfg::GraphsDFG, label::Symbol)::DFGVariable
+function deleteVariable!(dfg::GraphsDFG, label::Symbol)#::Tuple{AbstractDFGVariable, Vector{<:AbstractDFGFactor}}
     if !haskey(dfg.labelDict, label)
         error("Variable label '$(label)' does not exist in the factor graph")
     end
+
+    deleteNeighbors = true # reserved, orphaned factors are not supported at this time
+    if deleteNeighbors
+        neigfacs = map(l->deleteFactor!(dfg, l), getNeighbors(dfg, label))
+    end
+
+
     variable = dfg.g.vertices[dfg.labelDict[label]].dfgNode
     delete_vertex!(dfg.g.vertices[dfg.labelDict[label]], dfg.g)
     delete!(dfg.labelDict, label)
-    return variable
+    return variable, neigfacs
 end
 
 function deleteFactor!(dfg::GraphsDFG, label::Symbol)::DFGFactor

src/LightDFG/services/LightDFG.jl

@@ -130,14 +130,19 @@ function updateFactor!(dfg::LightDFG, factor::F)::F where F <: AbstractDFGFactor
     return factor
 end
 
-function deleteVariable!(dfg::LightDFG, label::Symbol)::AbstractDFGVariable
+function deleteVariable!(dfg::LightDFG, label::Symbol)#::Tuple{AbstractDFGVariable, Vector{<:AbstractDFGFactor}}
     if !haskey(dfg.g.variables, label)
         error("Variable label '$(label)' does not exist in the factor graph")
     end
+
+    deleteNeighbors = true # reserved, orphaned factors are not supported at this time
+    if deleteNeighbors
+        neigfacs = map(l->deleteFactor!(dfg, l), getNeighbors(dfg, label))
+    end
     variable = dfg.g.variables[label]
     rem_vertex!(dfg.g, dfg.g.labels[label])
 
-    return variable
+    return variable, neigfacs
 end
 
 function deleteFactor!(dfg::LightDFG, label::Symbol)::AbstractDFGFactor

src/services/AbstractDFG.jl

@@ -226,7 +226,7 @@ end
     $(SIGNATURES)
 Delete a DFGVariable from the DFG using its label.
 """
-function deleteVariable!(dfg::G, label::Symbol)::AbstractDFGVariable where G <: AbstractDFG
+function deleteVariable!(dfg::AbstractDFG, label::Symbol)#::Tuple{AbstractDFGVariable, Vector{<:AbstractDFGFactor}}
     error("deleteVariable! not implemented for $(typeof(dfg))")
 end
 """
@@ -389,7 +389,7 @@ end
     $(SIGNATURES)
 Delete a referenced DFGVariable from the DFG.
 """
-function deleteVariable!(dfg::G, variable::V)::AbstractDFGVariable where {G <: AbstractDFG, V <: AbstractDFGVariable}
+function deleteVariable!(dfg::AbstractDFG, variable::AbstractDFGVariable)#::Tuple{AbstractDFGVariable, Vector{<:AbstractDFGFactor}}
     return deleteVariable!(dfg, variable.label)
 end
 

test/LightDFGSummaryTypes.jl

@@ -17,6 +17,7 @@ v3 = VARTYPE(:c)
 f0 = FACTYPE(:af1)
 f1 = FACTYPE(:abf1)
 f2 = FACTYPE(:bcf1)
+append!(f2._variableOrderSymbols, [:b,:c])
 
 union!(v1.tags, [:VARIABLE, :POSE])
 union!(v2.tags, [:VARIABLE, :LANDMARK])

test/iifInterfaceTests.jl

@@ -66,14 +66,14 @@ end
     @test updateFactor!(dfg2, f2) == f2
     @test_throws ErrorException addFactor!(dfg2, [:b, :c], f2)
 
-    dv3 = deleteVariable!(dfg2, v3)
+    dv3, dv3facs = deleteVariable!(dfg2, v3)
     #TODO write compare if we want to compare complete one, for now just label
     # @test dv3 == v3
     @test dv3.label == v3.label
     @test_throws ErrorException deleteVariable!(dfg2, v3)
 
     @test issetequal(ls(dfg2),[:a,:b])
-    df2 = deleteFactor!(dfg2, f2)
+    df2 = dv3facs[1]
     #TODO write compare if we want to compare complete one, for now just label
     # @test df2 == f2
     @test df2.label == f2.label

test/interfaceTests.jl

@@ -44,6 +44,13 @@ end
     BigDataEntriesTestBlock!(fg1, var2)
 end
 
+# fg = fg1
+# v1 = var1
+# v2 = var2
+# v3 = var3
+# f0 = fac0
+# f1 = fac1
+# f2 = fac2
 @testset "TODO Sorteer groep" begin
     testGroup!(fg1, var1, var2, fac0, fac1)
 end

test/testBlocks.jl

@@ -311,6 +311,9 @@ function  DFGFactorSCA()
     f1 = DFGFactor(f1_lbl, [:a,:b], gfnd, tags = f1_tags, solvable=0)
 
     f2 = DFGFactor{TestCCW{TestFunctorInferenceType1}, Symbol}(:bcf1)
+    #TODO add tests for mutating vos in updateFactor and orphan related checks.
+    # we should perhaps prevent an empty vos
+    f2._variableOrderSymbols = [:b, :c]
 
     @test getLabel(f1) == f1_lbl
     @test getTags(f1) == f1_tags
@@ -406,14 +409,27 @@ function  VariablesandFactorsCRUD_SET!(fg, v1, v2, v3, f0, f1, f2)
         @test getFactor(fg, :bcf1) |> getTimestamp == newtimestamp
     end
     #deletions
-    @test getVariable(fg, :c) == deleteVariable!(fg, v3)
+    delvarCompare = getVariable(fg, :c)
+    delfacCompare = getFactor(fg, :bcf1)
+    delvar, delfacs = deleteVariable!(fg, v3)
+    @test delvarCompare == delvar
+    @test delfacCompare == delfacs[1]
     @test_throws ErrorException deleteVariable!(fg, v3)
-    @test issetequal(ls(fg),[:a,:b])
+    @test setdiff(ls(fg),[:a,:b]) == []
+
+    @test addVariable!(fg, v3) === v3
+    @test addFactor!(fg, f2) === f2
 
     @test getFactor(fg, :bcf1) == deleteFactor!(fg, f2)
     @test_throws ErrorException deleteFactor!(fg, f2)
     @test lsf(fg) == [:abf1]
 
+    delvarCompare = getVariable(fg, :c)
+    delfacCompare = []
+    delvar, delfacs = deleteVariable!(fg, v3)
+    @test delvarCompare == delvar
+    @test delfacCompare == []
+
     @test getVariable(fg, :a) == v1
     @test getVariable(fg, :a, :default) == v1