-
Notifications
You must be signed in to change notification settings - Fork 29
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
The solver will panic if a goal or constraint reduces to a constant expression #77
Comments
I don't know enough about the project to say for sure, but I think the problem traces to This sequence happens in my first example because adding the objective has let mut simple_expr = expr_arena.clone();
let _ = simple_expr.simplify().split_off_constant();
self.obj_expr_arena = Some(simple_expr); After the first line, LpExpression {
root: 0,
arena: [],
}, (equivalent to Later, when running the solver, the expression is formatted, which calls simplify again. This time, I bet something similar happens in my second example with the literal constraint. I don't know if this would break anything, but it seems that replacing the line |
+1. I don't know enough to comment on the proposed fix, but I believe we are stuck on what appears to be the same issue, where perfectly valid problems cause panics inside the LP Modeler simplification code. I've observed many cases where small (seemingly insignificant) tweaks to the problem can either cause or alleviate the panic. |
[Update] I was finally able to reduce my issue down to a small test case. It is basically the same as the one @zeta12ti proposed. The one caveat I will say is that I don't think it's as broad as any "literal expression" -- I think it's only related to literal 0.0. It would be great if the library could properly handle these cases - obviously it's possible to work around in client code but that starts to defeat the purpose of a library like this. This works fine:
This panics:
|
To be precise, there's a panic when the simplified constraint or goal doesn't contain any variables. For But it works for other literals too, not just let mut problem = LpProblem::new("Test", LpObjective::Maximize);
let x1 = LpContinuous::new("x1_0_X").lower_bound(0_f32).upper_bound(1000000.0);
let x2 = LpContinuous::new("x2_0_X").lower_bound(0_f32).upper_bound(1000000.0);
problem += -1.0 * (&x1) + 1.0 * (&x2);
problem += LpExpression::literal(1.1).ge(1.1); // note the change
CbcSolver::new().run(&problem).unwrap(); panics just the same. |
What should the behaviour be? A literal is not a valid constraint, do you suggest for interpreting that as a |
I made my suggestion in the second comment: when splitting off a constant, don't replace constant expressions with an invalid expression, but rather with zero.
What do you mean by that? Are you saying that
That would work (I think), but it's not necessary. For goals, that probably shouldn't be done. Either a constant goal should be provided to the solver (and then the solver (edit: by which I mean e.g. CBC Solver) is free to complain if it thinks that's a problem), or the library should explicitly return an error that can be caught and recovered from.
The library doesn't (currently) give any way to check if an expression simplifies to a constant. If this isn't fixed, the library should give a better error message than |
My opinion is the library should be able to seamlessly handle these cases. Mathematically, there is nothing special or magic about a coefficient of "0.0". The full context of my example above is that coefficients are actually being fed to the problem from an external source (database), not hard-coded inline in the source code... so what we observed in practice were sporadic library panics on some problems, which required quite a bit of debugging time to narrow down the issue. I have since gone through my code and put a bunch of "if coef.abs() > 0" statements to avoid adding these problematic constraints. This isn't a general solution but worked around the issue for me. I agree with @zeta12ti -- if this isn't going to be fixed, a clear and precise error message about the issue would be a good idea, and I'd also suggest adding ample warnings in the documentation as well. |
UPDATE: it seems the actual problem is literal expressions. Empty sums work fine, but they return a literal zero.
If a problem has an
LpExpression::literal
and no variables in a constraint or objective, trying to solve it will cause a panic.Here's a reproduction:
The backtrace is
Even if the problem is otherwise perfectly valid, adding such a constraint causes a panic.
The text was updated successfully, but these errors were encountered: