Skip to content
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

HalfspaceToRandomBayesianFactor fails without non-negative constraints #98

Open
rcabanasdepaz opened this issue Oct 11, 2021 · 5 comments
Assignees
Labels
bug Something isn't working

Comments

@rcabanasdepaz
Copy link
Collaborator

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.


        double[][] coef = ArraysUtil.reshape2d(new double[]{1,0,0,1}, 2,2);
        double[] vals0 = new double[]{0,1};
        double[] vals1 = new double[]{1,0};


        SeparateHalfspaceFactorFactory ff = SeparateHalfspaceFactorFactory
                .factory()
                .domain(Strides.as(0,2), Strides.as(1,2));


        ff.constraint(coef[0], Relationship.EQ, vals0[0], 0);
        ff.constraint(coef[1], Relationship.EQ, vals0[1], 0);
        ff.constraint(coef[0], Relationship.EQ, vals1[0], 1);
        ff.constraint(coef[1], Relationship.EQ, vals1[1], 1);

        // Non-negativeness constraints
        if(false) {
            ff.constraint(new double[]{1, 0}, Relationship.GEQ, 0.0, 0);
            ff.constraint(new double[]{0, 1}, Relationship.GEQ, 0.0, 0);
            ff.constraint(new double[]{1, 0}, Relationship.GEQ, 0.0, 1);
            ff.constraint(new double[]{0, 1}, Relationship.GEQ, 0.0, 1);
        }

        SeparateHalfspaceDefaultFactor f = (SeparateHalfspaceDefaultFactor) ff.get();
        f.printLinearProblem();

        // This works without non-neg constraints
        BayesianFactor bf1 = new SeparateLinearToRandomBayesian().apply(f, 0);
        System.out.println(bf1);

        // This does not work (unfeasible solution)
        BayesianFactor bf2 = new HalfspaceToRandomBayesianFactor().apply(f,0);
        System.out.println(bf2);

@cbonesana, @davidhuber , what do you think?

@rcabanasdepaz rcabanasdepaz added the bug Something isn't working label Oct 11, 2021
@rcabanasdepaz rcabanasdepaz self-assigned this Oct 11, 2021
@cbonesana
Copy link
Member

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.

@rcabanasdepaz
Copy link
Collaborator Author

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());
	}

@cbonesana
Copy link
Member

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 :)

@rcabanasdepaz
Copy link
Collaborator Author

Ok, in that case let's do no use HalfspaceToRandomBayesianFactor as a quick temporal solution, and leave as a future task fixing it.

@cbonesana
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants