-
Notifications
You must be signed in to change notification settings - Fork 2
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
variable solver data concept #231
Conversation
will look and answer through review, thanks for getting this going! |
Consolidate, drop buildSubFG_SPECIAL
save and load to gzip as default
add getVariablePPE and deprecated
fix fatal compilation errors
dont change directory for save tar
Codecov Report
@@ Coverage Diff @@
## 4Q19/poc/v0_6 #231 +/- ##
=================================================
- Coverage 68.78% 67.15% -1.64%
=================================================
Files 39 41 +2
Lines 2079 2195 +116
=================================================
+ Hits 1430 1474 +44
- Misses 649 721 +72
Continue to review full report at Codecov.
|
label::Symbol | ||
timestamp::DateTime | ||
tags::Vector{Symbol} | ||
estimateDict::Dict{Symbol, <:AbstractPointParametricEst} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
latest v0.5.3 now has this as ppeDict
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see here:
ppeDict::Dict{Symbol, <:AbstractPointParametricEst} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will likely fall out naturally when merging all this to master, just ask to keep an eye on it when we do please.
|
||
Return the estimates for a variable. | ||
""" | ||
getEstimates(v::VariableDataLevel1) = v.estimateDict |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This too has been deprecated, see master branch.
# | ||
# buildSubgraphFromLabels!, _copyIntoGraph!, getVariableIds | ||
# """ | ||
# function buildSubgraphFromLabels!_SPECIAL(dfg::G, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
""" | ||
function getVariableSolverData(dfg::AbstractDFG, variablekey::Symbol, solvekey::Symbol=:default) | ||
error("not implemented") | ||
return deepcopy(solverData(getVariable(dfg, variablekey), solvekey)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why a deepcopy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should verb get
not return the original memory object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think you are correct.
|
||
#var.solverDataDict[solvekey] = vnd | ||
|
||
setSolverData(var, deepcopy(vnd), key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see updated api in master to include an !
|
||
#var.solverDataDict[solvekey] = deepcopy(vnd) | ||
#for InMemoryDFGTypes, cloud would update here | ||
setSolverData(var, deepcopy(vnd), key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, why do a deep copy here? You think the user might accidentally point the code to the same vnd object and therefore become incorrect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should maybe leave the ‘deepcopy’ responsibility to the user.
error("VariableNodeData '$(solvekey)' already exists") | ||
end | ||
|
||
vnd = pop!(var.solverDataDict, solvekey) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one seems right to me, will think on it more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
requesting changes based on the first deepcopy -- should the verb get
not return the original memory object as standard behaviour?
Closing because of worry about merges. I will cherry pick or manually copy the relevant code. |
draft to help with the discussion in #182 concept