-
Notifications
You must be signed in to change notification settings - Fork 121
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
Error when solving non-DCP problems #523
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #523 +/- ##
==========================================
+ Coverage 92.26% 92.30% +0.03%
==========================================
Files 86 86
Lines 5300 5301 +1
==========================================
+ Hits 4890 4893 +3
+ Misses 410 408 -2 ☔ View full report in Codecov by Sentry. |
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 an alternative suggestion: that instead of a warning, we emit a hard error. This is what CVX(py) does.
I can see so many people ignoring the warnings. Convex is for DCP. There are plenty of other options if people don't want to use DCP.
That's a good idea, I've made those changes here. I've still left an opt-out in case there are rare cases in which it can be useful. I chose not to mention the opt-out in the error message since I don't want folks turning to it without understanding all the implications. |
src/Convex.jl
Outdated
|
||
Convex.emit_dcp_warnings() = false | ||
to (globally) allow non-DCP problems to be solved. Note that if a problem is not DCP, the solution obtained by Convex.jl can be wildly incorrect (even if the solution status is `OPTIMAL`!). |
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.
Is there a practical use-case for allowing this? cc @mlubin
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 remember someone using it for alternating minimization. It could be something where they were going to fix a variable but hadn’t yet and the error occurs too early (when you construct the expression not when you solve the problem). The early errors should give more useful stacktraces 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.
actually something weird with the current place the errors are thrown is that it is only when you call vexity
. This happens during problem solving so we at least always throw then. But it doesn't happen when you construct the expression, unless you show
it, which does check the vexity (to print it). So I think this isn't ideal, since you end up with different-seeming behavior in the REPL (as it shows things).
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.
What about just erroring on optimize!
?
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.
ok, I've made that change, and removed the opt-out
Co-authored-by: Oscar Dowson <odow@users.noreply.github.com>
docs/examples_literate/supplemental_material/Convex.jl_intro_ISMP2015.jl
Outdated
Show resolved
Hide resolved
src/solution.jl
Outdated
@@ -52,6 +52,10 @@ function solve!(p::Problem, optimizer_factory; silent_solver = false) | |||
MOI.set(model, MOI.Silent(), true) | |||
end | |||
|
|||
if vexity(p) == NotDcp() |
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 think this isn’t quite right actually, we need to check for eg concave minimization problems
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.
Or at least, this is right, but there are other cases.
Noticed
while running the tests for #522. Using method invalidation for this is silly as there's no reason to recompile code; checking a global ref is very fast compared to everything else Convex is doing. Also, this fix is non-breaking as the previous advice to invalidate the method will continue to work here.