Skip to content

Regression, new bug, related to useMsgLikelihoods #1574

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

Closed
dehann opened this issue Jul 26, 2022 · 7 comments · Fixed by #1576
Closed

Regression, new bug, related to useMsgLikelihoods #1574

dehann opened this issue Jul 26, 2022 · 7 comments · Fixed by #1576
Labels
bug CI previously Travis inference
Milestone

Comments

@dehann
Copy link
Member

dehann commented Jul 26, 2022

Looks like it has something to do with getSolverParams(fg).useMsgLikelihoods = true. MWE:

using IncrementalInference

fg = generateGraph_CaesarRing1D()
getSolverParams(fg).useMsgLikelihoods = true

solveGraph!(fg)

Code completes fine if useMsgLikelihoods=false

Originally posted by @dehann in #1572 (comment)

@dehann dehann added bug inference CI previously Travis labels Jul 26, 2022
@dehann
Copy link
Member Author

dehann commented Jul 26, 2022

Was first seen in testDefaultDeconv.jl

@dehann dehann added this to the v0.30.1 milestone Jul 26, 2022
dehann added a commit that referenced this issue Jul 26, 2022
@dehann
Copy link
Member Author

dehann commented Jul 26, 2022

This is the printout:

!(z isa Vector{Float64}) && (@warn "H" z x1 x2)

@Affie, what does the "H" mean? Looks like the way to read this line of code is that z should be a Vector{Float64} -- so being a ::Matrix is bad?

┌ Warning: H
│ z = 1×1 Matrix{Float64}: …
│ x1 = 1-element Vector{Float64}: …
│ x2 = 1-element Vector{Float64}: …
└ @ IncrementalInference ~/.julia/dev/IncrementalInference/src/Factors/LinearRelative.jl:41

@dehann
Copy link
Member Author

dehann commented Jul 26, 2022

So here is one inconsistency, sampleFactor returns different types depending on whether the factor was created by the user or from a tree likelihood message.

This is good (returns Vector{Flpat64}):

@test z isa Vector{<:Real}

But then just lower down this, which is bad (returns Matrix{Float64}):

@test z isa Vector{<:Real}

@dehann
Copy link
Member Author

dehann commented Jul 26, 2022

Ah, that's because the differential factor from the tree likelihood factor is an MKD (not Normal like user factors) which uses a different getSample function. There is some inconsistency there.

@dehann
Copy link
Member Author

dehann commented Jul 26, 2022

Okay so sampleTangent on ::Normal returns a ::Vector:

X = sampleTangent(M, fT.Z)
@test X isa Vector{<:Real}

while sampleTangent on ::MKD returns a ::Matrix:

X = sampleTangent(M, fT.Z)
@test X isa Vector{<:Real}

@dehann
Copy link
Member Author

dehann commented Jul 26, 2022

So fixed with forced type change of sample return for MKD case. If matrix has just one column, then convert to a vector:
71de84e

@Affie
Copy link
Member

Affie commented Jul 26, 2022

KDE returns a matrix on sample and it is used as coordinates which should be a vector, a recent update in Manifolds.jl showed this error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug CI previously Travis inference
Projects
None yet
2 participants