Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

updateVariableSolverData! bug and API #565

Open
Affie opened this issue Jul 29, 2020 · 20 comments
Open

updateVariableSolverData! bug and API #565

Affie opened this issue Jul 29, 2020 · 20 comments

Comments

@Affie
Copy link
Member

Affie commented Jul 29, 2020

MethodError: no method matching updateVariableSolverData!(::LightDFG{SolverParams,DFGVariable,DFGFactor}, ::Symbol, ::Bool, ::Array{Symbol,1}, ::Bool)

and

MethodError: no method matching  updateVariableSolverData!(::LightDFG{SolverParams,DFGVariable,DFGFactor}, ::Symbol, ::Nothing, ::Symbol, ::Bool)

and this is also broken:

updateVariableSolverData!(dfg, xi.label, getSolverData(xi, :default), :graphinit, true, Symbol[], false)
@Affie Affie added the bug Something isn't working label Jul 29, 2020
@Affie Affie added this to the v0.10.0 milestone Jul 29, 2020
@Affie
Copy link
Member Author

Affie commented Jul 29, 2020

Add tests for this in DFG

@Affie Affie changed the title MethodError: updateVariableSolverData! MethodError: updateVariableSolverData! and broken functionality Jul 29, 2020
@Affie Affie modified the milestones: v0.10.0, v0.0.x Aug 12, 2020
@Affie
Copy link
Member Author

Affie commented Aug 12, 2020

@warn "TODO It looks like solverKey as parameter is deprecated, set it in vnd, or keep this function?"

We should decide how we want to use this.

@Affie
Copy link
Member Author

Affie commented Aug 12, 2020

originally posted in a comment: #570 (comment)

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  

@dehann
Copy link
Member

dehann commented Aug 12, 2020

Maybe we shouldn't overwrite expected behaviour from base copy with deepcopy?

definitely not change the behaviour of a method from Base.

@dehann
Copy link
Member

dehann commented Aug 12, 2020

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.

@dehann
Copy link
Member

dehann commented Aug 12, 2020

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.

@Affie
Copy link
Member Author

Affie commented Aug 12, 2020

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

@dehann
Copy link
Member

dehann commented Aug 12, 2020

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))

@Affie
Copy link
Member Author

Affie commented Aug 12, 2020

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.

@dehann
Copy link
Member

dehann commented Aug 12, 2020

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.

@Affie
Copy link
Member Author

Affie commented Aug 12, 2020

I'll try and find it. it is where the init solve key is made

https://github.com/JuliaRobotics/IncrementalInference.jl/blob/3252763e99299d2657a2390da3afbe004739ec78/src/FactorGraph.jl#L842

@dehann dehann modified the milestones: v0.0.x, v0.10.0 Aug 12, 2020
@dehann
Copy link
Member

dehann commented Aug 12, 2020

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

@dehann
Copy link
Member

dehann commented Aug 12, 2020

remember the user of VND is probably unaware that there is duplication of solveKey.

@Affie
Copy link
Member Author

Affie commented Aug 12, 2020

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.

@Affie
Copy link
Member Author

Affie commented Aug 12, 2020

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

@dehann
Copy link
Member

dehann commented Aug 12, 2020

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

@dehann
Copy link
Member

dehann commented Aug 12, 2020

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! #565 (comment).

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 (#565 (comment)).

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 (#565 (comment)) 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))

@dehann dehann removed this from the v0.10.0 milestone Aug 13, 2020
@dehann dehann added this to the v0.11.0 milestone Aug 13, 2020
@dehann
Copy link
Member

dehann commented Aug 13, 2020

FYI:

"""
$SIGNATURES
Duplicate a supersolve (solverKey).
"""
function deepcopySolvekeys!(dfg::AbstractDFG,
dest::Symbol,
src::Symbol;
solvable::Int=0,
labels=ls(dfg, solvable=solvable),
verbose::Bool=false )
#
for x in labels
sd = deepcopy(getSolverData(getVariable(dfg,x), src))
sd.solverKey = dest
updateVariableSolverData!(dfg, x, sd, true, Symbol[], verbose )
end
end
const deepcopySupersolve! = deepcopySolvekeys!

@dehann dehann changed the title MethodError: updateVariableSolverData! and broken functionality updateVariableSolverData! bug and API Aug 13, 2020
@dehann dehann added the vote label Aug 13, 2020
@Affie
Copy link
Member Author

Affie commented Aug 13, 2020

deepcopySolvekeys! I didn't know about that one. (or forgot) it similar to my suggestion of deepcopySolverData. I would say either use deepcopySolvekeys! or #565 (comment)
No need to make another similar function.

@Affie Affie modified the milestones: v0.11.0, v0.12.0 Dec 4, 2020
@dehann dehann modified the milestones: v0.12.0, v0.13.0 Feb 13, 2021
@dehann dehann modified the milestones: v0.15.0, v0.0.x Jun 29, 2021
@Affie
Copy link
Member Author

Affie commented Aug 28, 2024

This is somewhat related to #1084, maybe both will use the same new verb.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants