-
-
Notifications
You must be signed in to change notification settings - Fork 390
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
Fix helpful error #1366
Fix helpful error #1366
Conversation
Should we throw a proper exception type to distinguish the helpful error message from the un-helpful "S not defined"? Because this wasn't caught in tests as it still throws an error. |
Can't we just make this work? if var isa Number && !(value isa Number)
parse_one_operator_variable(_error, infoexpr, Val(reverse_sense(sense)), esc_nonconstant(var))
return value
end |
I don't understand why it was passing the tests since we check that its an |
The question is whether it is more or less confusing for users. Then you have to say why @variable(m, 0 <= x) works but a = 0
@variable(m, a <= x) I can see it leading to more confusion than "it never works" |
We should at least throw a warning because it's going to fail badly when it's not constant. |
The confusion always seems to be with @variable(m, 0 <= x) I've yet to see a complaint about a = 0
@variable(m, a <= x) |
Okay lets change it. |
Codecov Report
@@ Coverage Diff @@
## master #1366 +/- ##
==========================================
+ Coverage 89.44% 89.51% +0.06%
==========================================
Files 24 24
Lines 3412 3413 +1
==========================================
+ Hits 3052 3055 +3
+ Misses 360 358 -2
Continue to review full report at Codecov.
|
I have changed it. It now works instead of erroring :) |
src/macros.jl
Outdated
@@ -965,18 +965,22 @@ function parsevariable(_error::Function, infoexpr::VariableInfoExpr, sense::Symb | |||
# Variable declaration of the form: var sense value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is out of date, so are the argument names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I explains where the function arguments come from. We should probably rename to lhs rhs though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have refactored it in two methods
test/variable.jl
Outdated
@test isequal(mcon[:ubonly],ubonly) | ||
@variable(mcon, ubonly1 <= 1, Int) | ||
@test isequal(mcon[:ubonly1],ubonly1) | ||
@variable(mcon, 1 >= ubonly2, Int) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make a separate test for this.
Closes #1365