Open Affie opened 4 years ago
Add tests for this in DFG
We should decide how we want to use this.
originally posted in a comment: https://github.com/JuliaRobotics/DistributedFactorGraphs.jl/pull/570#discussion_r464301676
Maybe we shouldn't overwrite expected behaviour from base copy with deepcopy?
I expect copy
to return a shallow copy and deepcopy for this.
Personally I don't think we should provide too many functions. Every function has to be documented and maintained. The user can rather just do
dcvnd = deepcopy(vnd)
dcvnd.selverKey = newKey
Is the function we are trying to replace updateSolverData!(dfg, label, vnd, newsolvekey)
?
Where vnd.solvekey != newsolvekey
.
Perhaps something like (although IIF needs to be updated slightly for it):
function deepcopySolverData!(dfg::AbstractDFG, label::Symbol, srcKey::Sybol, dstKey::Symbol)
vnd = deepcopy(getSolverData!(dfg, label, srcKey))
vnd.solverKey = dstKey
updateSolverData!(dfg, vnd)
end
Now that I'm thinking it through. update
is maybe not the correct function to use. Its adding new solverdata so maybe:
function addSolverData!(dfg::AbstractDFG, label::Symbol, vnd, solverKey::Symbol)
vnd.solverKey = dstKey
addSolverData!(dfg, vnd)
end
Maybe we shouldn't overwrite expected behaviour from base copy with deepcopy?
definitely not change the behaviour of a method from Base.
Is the function we are trying to replace updateSolverData!(dfg, label, vnd, newsolvekey)?
No, the idea is just to have the solveKey available inside the structure as a duplicate from the Dict container in which the VND is stored. Nothing crazy, just duplication.
Perhaps something like (although IIF needs to be updated slightly for it):
I disagree with the user having to do band-aids for DFG internal duplication decision.
update is maybe not the correct function to use. Its adding new solverdata so maybe:
update
verb will need to update both registers of the duplication.
No, the idea is just to have the solveKey available inside the structure as a duplicate from the Dict container in which the VND is stored. Nothing crazy, just duplication.
Dehann, I think you are missing the point
maybe, how I understand it is that Sam asked for a duplicate of the solveKey inside VND. So nothing changes, except that there is a forced duplication of one ::Symbol register.
Dict{Symbol, VND}( mySolveKey => VND(... solveKey=mySolveKey))
No that is not the issue. The issue is it was used in IIF to copy from one solver data to another. and the key was removed from the parameters completely. I thought to add copy functionality.
I thought to add copy functionality.
not following
The issue is it was used in IIF to copy from one solver data to another.
Do you perhaps have the piece of code where that is happening? Links above are only to DFG.
I'll try and find it. it is where the init solve key is made
Okay, so that line in IIF is using the update
verb to insert a new copy. So in that specific use case in IIF update
is fine, and IIF is responsible for making a deepcopy
of the VND
data.
I would add---to address the duplication of solveKey
in VND---that the parameter argument passed to the update
function takes precedence. So when a user updates a VND, then DFG should check that the duplicated registers match, and discrepancies are not introduced:
vnd = getVND(..., :default)
dcvnd = deepcopy(vnd) # solveKey=:default
updateVariableSolverData!(... dcvnd, , :graphinit, ...)
In this case, DFG should update dcvnd.solveKey = :graphinit
.
PS, I think solverKey
is a spelling mistake too (xref #599 , #602 ).
remember the user of VND is probably unaware that there is duplication of solveKey
.
My and Sam's vote is for
vnd = getVND(..., :default)
dcvnd = deepcopy(vnd) # solveKey=:default
vnd.solverKey = :graphinit
addVariableSolverData!(..., dcvnd)
Just because ist a little strange to have updateVariableSolverData!
sometimes modifying your vnd.
PS, I think solverKey is a spelling mistake too.
I saw there was a bunch of solvekey and a few solverkey mixed. I think Sam went with solverkey
i disagree, this is a special case and would rather have:
updateVariableSolverData!(..., dcvnd, :newkey, checkSolvekey=false)
since the verb is update
.
A DFG design decision/requirement is to duplicate solveKey
inside VND, so DFG should be responsible for handling that potential discrepancy (case by case and according to precedence). Seems pretty clear that the function call argument :newkey
from the user is the most important element here. We can also document/explain checkSolvekey=true
properly.
The quirk about duplicating solveKey
is to tell a user where the VND object came from, the emminant instance of solveKey is still the key in the ::Dict{Symbol, VND}
container -- at least that is the whole point of all the key => value
pairs construct.
Using the duplicated VND.solveKey
as precedence on where a VND object should be stored seems weird to me, i.e. key => VND(**key2**,...)
. Otherwise we should always (or clearly define when to) use that pattern where all objects internally keep their destination key, and then you apply that object to a container:
obj.key = :newkey
add|updateObject!(container, obj)
Doesn't make sense, all the get
methods must have solveKey
as argument, so the most natural seems that the set
methods also use an argument. obj.key = :getlocation; obj = getObj!(container, obj)
doesn't make sense, so why "standardize" around obj.key = :somewhere; setObj!(container, obj)
. All the code in DFG/IIF/RoME/RoMEPlotting currently uses get|setObj[!](container, [obj,] solveKey=:default)
. Quick Find All on my workspace shows 214 instances of solveKey.
Similarly, in IIF we want to compare to solveKey
values it would be really clunky to carry around both VND's to do the comparison -- its more natural to use a "key" to get at something, rather than locking a key inside a box and telling the driver, okay please open the box to get the key where to put the box and don't worry about all the other stuff also floating around inside the box.
There is a special interest design consideration inside DFG only to have this duplication, and should probably not be forced down on users. If you don't know about the VND.solveKey
duplication then you are bound to make mistakes. If you know about the duplication and want to force a discrepancy, then use a special keyword checkSolvekey
to suppress the "automation".
okay, so i thought some more and came up with a compromise (especially since i’m out voted 2-1). Why not just do both. Do as Johan and Sam suggest by extracting VND.solveKey
on update!
https://github.com/JuliaRobotics/DistributedFactorGraphs.jl/issues/565#issuecomment-673038085.
Then also write a second update! wrapper function with different parameter arguments which includes the ::Symbol
which first updates the VND.solveKey
assuming a keyword checkSolvekey=true
and then calls the first update!
without the additional ::Symbol
(https://github.com/JuliaRobotics/DistributedFactorGraphs.jl/issues/565#issuecomment-673039977).
Just to reiterate, literally every other time (and there are many) that we use solveKey, the solveKey is passed in as a argument to the function. So update!
will be a special case. I would use the latter wrapper function to keep the API use consistent.
NOTE, the 2-1 suggested approach (https://github.com/JuliaRobotics/DistributedFactorGraphs.jl/issues/565#issuecomment-673038085) is more consistent with the example in the repo README, where the key values are stored in DFGVariable
object, and the dfg container key values are just driven by var.label
:
# In-memory DFG
dfg = LightDFG{NoSolverParams}()
addVariable!(dfg, DFGVariable(:a))
addVariable!(dfg, DFGVariable(:b))
deepcopySolvekeys!
I didn't know about that one. (or forgot) it similar to my suggestion of deepcopySolverData
. I would say either use deepcopySolvekeys!
or https://github.com/JuliaRobotics/DistributedFactorGraphs.jl/issues/565#issuecomment-673038085
No need to make another similar function.
This is somewhat related to #1084, maybe both will use the same new verb.
and
and this is also broken: