Refactor graph copy and subgraph functions#373
Conversation
dehann
left a comment
There was a problem hiding this comment.
Punchline is there are two approaches to do a copy:
- mini-batch of all variables followed by all factors; or
- sequentially around each node in the graph (edge by edge).
Hi @GearsAD , is there a particular preference on approach for DB version? Johan is busy consolidating the various graph copy functions into one or two primary copy functions in DFG.
| LightDFG(FactorGraph{Int,DFGVariable,DFGFactor}(), description, userId, robotId, sessionId, userData, robotData, sessionData, Symbol[], solverParams) | ||
|
|
||
|
|
||
| LightDFG{T,V,F}(description::String, |
There was a problem hiding this comment.
should this not perhaps be an inner constructor, or does it not matter ( I'm not sure about the best practices on inner or outer constructors at the moment)?
There was a problem hiding this comment.
https://docs.julialang.org/en/v1/manual/constructors/#man-inner-constructor-methods-1
It is good practice to provide as few inner constructor methods as possible: only those taking all arguments explicitly and enforcing essential error checking and transformation. Additional convenience constructor methods, supplying default values or auxiliary transformations, should be provided as outer constructors that call the inner constructors to do the heavy lifting. This separation is typically quite natural.
|
I think we can merge this, right? This is very cool, thanks @Affie ! |
|
LGTM, just one thing - is there any reason (other than the original function signatures) that we combine variables and factors as input to This just results in the lists being split up first thing into the function. If there isn't a reason beyond the initial design, I think we should split it up to make the calls more efficient. |
|
Btw, I meant that we can do that in a subsequent PR - I'll add that. This is good thanks. |
|
Oh I see you add it to the list already :) If you haven't started, let's save it for another PR |
|
Ok, will do separate PR. Just don't start using it in master yet. |
Close #300
Still to do:
buildSubgraph_copyIntoGraphin favor ofcopyGraph/deepcopyGraph/deepcopyGraph!getSubgraphin favor ofbuildSubgraphgetSubgraphAroundNodein favor ofbuildSubgraph