WIP on manifold optimization and factors #1292
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1292 +/- ##
===========================================
- Coverage 40.63% 10.63% -30.00%
===========================================
Files 57 57
Lines 4794 4860 +66
===========================================
- Hits 1948 517 -1431
- Misses 2846 4343 +1497
Continue to review full report at Codecov.
|
|
|
||
| for cfp in cfvec | ||
|
|
||
| varOrder = getindex.(Ref(labeldict), cfp.varOrder) |
There was a problem hiding this comment.
again, julia is already pass by reference, so adding a second tier Ref over a dict ref seems weird to me
src/ApproxConv.jl
Outdated
| function calcVariableCovarianceBasic(M::AbstractManifold, ptsArr::Vector{P}) where P | ||
| #TODO double check the maths,. it looks like its working at least for groups | ||
| μ = mean(M, ptsArr) | ||
| Xcs = vee.(Ref(M), Ref(μ), log.(Ref(M), Ref(μ), ptsArr)) | ||
| Σ = mean(Xcs .* adjoint.(Xcs)) | ||
| @debug "calcVariableCovarianceBasic" μ | ||
| @debug "calcVariableCovarianceBasic" Σ | ||
| # TODO don't know what to do here so keeping as before, #FIXME it will break | ||
| # a change between this and previous is that a full covariance matrix is returned | ||
| msst = Σ | ||
| msst_ = 0 < sum(1e-10 .< msst) ? maximum(msst) : 1.0 | ||
| return msst_ | ||
| end | ||
|
|
There was a problem hiding this comment.
@dehann, have a look please
There was a problem hiding this comment.
msst_ is probably part of initialization when trivial case errors occur in this calculation. I was expecting the first mean for mu, the coordinates on Xc look right (around mu). The adjoint is somewhat unfamiliar to me in this context, must just make sure that the communativity is right, e.g. hypothetically:
# good
aRc = adjoint(bRa)*bRc
# vs bad
bRa*adjoint(bRc)
since the latter would give incorrect answers. I'm not sure and would need to dig more to learn more.
There was a problem hiding this comment.
Xcs is in coordinates, and transpose is just used to build the matrix to be used for the covariance
There was a problem hiding this comment.
See #1280, we can perhaps continue there
|
HI @Affie , the error on serialization has to do with the new ccw.manifolds field: The issue is that during unpackFactor, we need to construct a ccw without yet knowing what the manifolds are from variables to which it will be attached, see the which creates the problem on the new line of code here (getManifolds on an empty array): IncrementalInference.jl/src/FactorGraph.jl Line 659 in 9718395 |
|
ill look more tomorrow |
|
About serialization issue of new manifolds field. Yes, I know it's caused by serialization. I just didn't know how to fix it. And skip it in the specific test for now. |
Tests added:
Sphere(2)and one prior fail #1293Needs:
Update MeanMaxPPE to a point on manifold DistributedFactorGraphs.jl#788Breaks (TODO):
Maybe CCW serialization- slightly related to Serialization and Marshaling (again) DistributedFactorGraphs.jl#590 (comment)vartypesfield in ccw. captured in Possible compareAll bug DistributedFactorGraphs.jl#434Parametric