Open rcabanasdepaz opened 3 years ago
From what I can see in the code, HalfspaceToRandomBayesianFactor
just uses first the HalfspaceToVertex
converter and then VertexToRandomBayesian
, while SeparateLinearToRandomBayesian
is a completely different implementation.
This issue could be caused by a wrong order in the Neighbourhood#random(GenericFactor)
checks for classes or by a design choice in the HalfspaceToVertex
converter.
Yes, this is solved if the following order is used:
private BayesianFactor random(GenericFactor factor) {
if (factor instanceof ExtensiveLinearFactor) {
return new ExtensiveLinearToRandomBayesian().apply((ExtensiveLinearFactor<?>) factor);
} else if (factor instanceof SeparateLinearFactor) {
return new SeparateLinearToRandomBayesian().apply((SeparateLinearFactor<?>) factor);
} else if (factor instanceof SeparateHalfspaceFactor) {
return new HalfspaceToRandomBayesianFactor().apply((SeparateHalfspaceFactor) factor);
} else if (factor instanceof BayesianFactor) {
return (BayesianFactor) factor;
}
throw new IllegalArgumentException("Unsupported class for random generation: " + factor.getClass());
}
No, this is not a valid sequence since factor instanceof SeparateHalfspaceFactor
will always be false
because SeparateHalfspaceFactor
extends SeparateLinearFactor
and the before check will trigger first.
At this point, the solutions are two: do not use the HalfspaceToRandomBayesianFactor
converter at all in this method or fix the converter to work as intended :)
Ok, in that case let's do no use HalfspaceToRandomBayesianFactor
as a quick temporal solution, and leave as a future task fixing it.
Then please do the same change for approxlp2
package in Neighbourhood#random(GenericFactor)
.
We know the two versions have very similar code, but the little differences didn't help us to find a way to unify them.
When running approxLP, in the method
ch.idsia.crema.inference.approxlp1.Neighbourhood::random
uses the converter HalfspaceToRandomBayesianFactor, which would fail if the H-factor does not contain the non-negative constraints. However, this is not the case SeparateLinearToRandomBayesian, which was indeed the converter used in older crema versions.Is there any advantage of HalfspaceToRandomBayesianFactor wrt SeparateLinearToRandomBayesian? If not, we might need to change the converter for allowing H-factors without non-negative constraints. Or at least, set a flag for controlling which one is used.
Here you have a simple code where one works and the other does not.
@cbonesana, @davidhuber , what do you think?