Implement TimeLimit #808
Implement TimeLimit #808
Conversation
|
I'll set it to |
Codecov Report
@@ Coverage Diff @@
## master #808 +/- ##
==========================================
- Coverage 94.21% 90.86% -3.35%
==========================================
Files 59 66 +7
Lines 6892 7441 +549
==========================================
+ Hits 6493 6761 +268
- Misses 399 680 +281
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
The tests do not check that the solver returns the TIME_LIMIT status when the time limit is hit. This is actually pretty challenging to do correctly because some solvers could solve a tiny problem very fast and return OPTIMAL before checking the time limit. However, it's worth a try or a TODO.
|
I made the following changes: TODO:
|
|
I made the following changes:
Still TODO:
|
| @@ -14,6 +14,7 @@ end | |||
| # `UniversalFallback` needed for `MOI.Silent` | |||
| mock = MOIU.MockOptimizer(MOIU.UniversalFallback(MOIU.Model{Float64}())) | |||
| MOI.set(mock, MOI.Silent(), true) | |||
| MOI.set(mock, MOI.TimeLimitSec(), nothing) | |||
There was a problem hiding this comment.
This is the default value, so why is the set needed?
There was a problem hiding this comment.
I think it should be remove. My bad.
There was a problem hiding this comment.
Well, removing that line breaks. nothing is the default value but I didn't set it in first place. I put that line back for now.
I don't know how to set a default value, and I think Silent() default value isn't set too. I'll be looking into it soon.
There was a problem hiding this comment.
What breaks? This needs to be resolved before we can merge.
There was a problem hiding this comment.
When get() is used on TimeLimitSec() instead of returning nothing as default value it throws an error. That happened in src/Test/UnitTests/attributes.jl:
KeyError: key MathOptInterface.TimeLimitSec() not found
There was a problem hiding this comment.
I still have to make changes in
src/Utilities/mockoptimizer.jlto set default values
Indeed, this requires the internal model to always support TimeLimitSec. You can either not set the default (like it is currently done for Silent) or only set it in case the internal model supports it (you can check that with supports).
I would rather do the first option (not setting the default) as the other behavior is somewhat weird, setting the optimizer attributes of ModelLike passed to MockOptimizer is a rather suprising behavior
There was a problem hiding this comment.
I think that would be strange to put default value is in docs but not testing for it because it's not implemented in MOI side.
I had issues with the use of supports since it returns an error instead of false for Silent() and TimeLimitSec():
https://github.com/JuliaOpt/MathOptInterface.jl/blob/ab2d0f05205a81c3848f9f1d870b75332b1bddd2/src/attributes.jl#L155-L169
To be honest I don't know how to solve that without some important changes as this bad try catch I did in last push.
There was a problem hiding this comment.
it's not implemented in MOI side
MockOptimizer is just a utility for testing so I don't think it is an issul if it does not have the default values for Silent and TimeLimitSec.
If new optimizer attributes are defined outside of MOI, MockOptimizer won't be able to have the right default values. It's case is a bit specific since it supports any optimizer attributes (when used with UniversalFallback), even some that may not defined in MOI. But it actually completely ignores their value.
That's not the case for solvers that only support specific attributes, take their value into account and should have the correct default values.
There was a problem hiding this comment.
My understanding is: if I remove default from MockOptimizer then MOI can't have a test to check if Optimizers implement defaults correctly. If I didn't understand that well then I don't see why MockOptimizer should have this feature but if I'm right then I believe this feature is quite important.
I'll push without the default values.
There was a problem hiding this comment.
if I remove default from
MockOptimizerthen MOI can't have a test to check if Optimizers implement defaults correctly.
You can set the attribute's value to its default before running the tests as it is done for Silent here:
https://github.com/JuliaOpt/MathOptInterface.jl/blob/42cdc3454cdb88345da0adb9cce2a57497c2fad4/test/Test/unit.jl#L15-L16
Of course actual solvers shouldn't do that but for the MockOptimizer it's ok to mock the test
|
I'll add a TODO in the code for now but I'm not sure where is the right place to add it since I didn't find any other test for Termination status. |
|
Looking good @remi-garcia. Sorry for the back and forth on the default options. |
|
I'm actually learning so much doing this so I really don't mind. Thank you for taking so much time helping me with this PR. |
|
@remi-garcia Thanks for implementing this! Just a quick question: does the time limit concerns the CPU time passed in the solver, or the real time? |
|
I think it would be better to use the CPU time passed in the solver |
|
I should have precise that in docs, sorry. Maybe we should precise that in |
|
For Mosek, it's unclear if it's CPUTime or REALTime (see https://docs.mosek.com/8.1/rmosek/parameters.html#mosek.dparam.optimizer_max_time). |
|
I understood the limit as wall clock time (real time). We need to clarify this in the docs. I'm not sure how essential it is to have a second solver-independent option with a CPU time limit, that's a bit esoteric. |
Implement TimeLimit using the example of Silent #695
Note: for the tests I used Integers TimeLimit because I didn't know the right way to@testa float value