feat: add various options to the scip solver

[KnorpelSenf]

• setting the verbosity level
• setting the heuristics
• setting the time limit
• setting the memory limit
• setting int, longint, bool, str, and float options

[lovasoa]

Great!

Could we:

• avoid panicking and return the error to the user instead?
• make a single generic set_option method? (with a trait implemented on the required types)
• add tests?

[lovasoa]

By the way, can I add you to the rust-or GitHub org? You have been doing a lot of great work in the space recently.

[KnorpelSenf]

Great!

Could we:

* avoid panicking and return the error to the user instead?

* make a single generic set_option method? (with a trait implemented on the required types)

* add tests?

Yep, I'll fix all that!

By the way, can I add you to the rust-or GitHub org? You have been doing a lot of great work in the space recently.

Absolutely, that would make things easier. Thanks!

[KnorpelSenf]

@lovasoa is that how you expected the trait to look?

Also, I have no idea how to test this properly. There is no way to get the option, so we can't really test if the option was set without solving a model and asserting that it was solved a certain way. That seems excessively expensive. How to proceed?

[KnorpelSenf]

Also, setting the time limit partially moves the model … that is not a very ergonomic API. I am not really sure how to improve this.

[lovasoa]

Yes, you should either take the SCIPProblem by value and return a SCIPProblem, or take it by mutable reference and return nothing (using std::mem::take or replace). Try to be consistent with option setting in other solvers.

[lovasoa]

for the trait, it looks good this way 👍

for testing, yes, you should make a problem, set an option, solve, and check that the option was respected by checking the solution

[KnorpelSenf]

Yes, you should either take the SCIPProblem by value and return a SCIPProblem, or take it by mutable reference and return nothing (using std::mem::take or replace). Try to be consistent with option setting in other solvers.

So we would do something like

    pub fn set_memory_limit(&mut self, memory_limit: usize) {
        std::mem::take(self.as_inner_mut()).set_memory_limit(memory_limit);
    }

or maybe

    pub fn set_memory_limit(mut self, memory_limit: usize) -> Self {
        std::mem::take(self.as_inner_mut()).set_memory_limit(memory_limit);
        self
    }

etc?

The only other place I found that has this is the set_parameter of of coin_cbc. However, in using good_lp, I always found it to be a bit annoying to use. In the README example, I want to do:

let solution = vars.maximise(10 * (a - b / 5) - b)
        .using(default_solver) // multiple solvers available
        .with(constraint!(a + 2 <= b))
        .with(constraint!(1 + a >= 4 - b))
        .set_parameter("name", "value")
        .solve()?;

Unfortunately, this isn't possible and now I have to do

let mut model = vars.maximise(10 * (a - b / 5) - b)
        .using(default_solver) // multiple solvers available
        .with(constraint!(a + 2 <= b))
        .with(constraint!(1 + a >= 4 - b));
model.set_parameter("name", "value");
let solution = model.solve()?;

which looks odd to me.

Should I still go ahead with the the former suggestion for the sake of consistency?

[KnorpelSenf]

for the trait, it looks good this way 👍

Nice! It inspired scipopt/russcip#196 btw

for testing, yes, you should make a problem, set an option, solve, and check that the option was respected by checking the solution

That will take a very long time, add a lot of complexity, and slow down the tests, too. I would have to

• create a model, set a short timeout, solve it, measure the time, and assert that the solver completed fast enough
• create a model, solve it, somehow capture the stdout of the process, and perform assertions based on what was logged
• create a model with a complexity that the four SCIP heuristics matter for the solving process, solve this model with all heuristics, and somehow assert that the right heuristics were used by the solver (that would have to be investigated)
• create a sufficiently complex model that it uses a lot of memory, set the memory limit option, measure the memory consumption of the current process, and assert that it did not exceed certain values
• for each untested type of option in the model (bool, longint, etc), create a model that is affected by this option and then make sure it is set correctly

This honestly sounds overkill when factoring in the low complexity of my changes. We would effectively be performing the exact same tests as russcip already runs, so we just test the underlying crate. Instead, shouldn't we just read back the parameters in the tests and make sure that they were set correctly? Reading model parameters is available in latest russcip@0.6.1 which we could update to.

[lovasoa]

Okay! let's go with option 2: read the parameters back.

[KnorpelSenf]

Thanks, I'll perform these changes soon!

Do you have an opinion on which API to use in #84 (comment)?

[lovasoa]

no opinion :)

[KnorpelSenf]

I'm back!

And I have a number of questions …

[KnorpelSenf]

I split this into two traits so that people can set a &str and get a String. Should we try to unify the two traits? If yes, how would we resolve this issue?

[lovasoa]

I think it's okay to implement it for &str only, if we don't need a String

[KnorpelSenf]

We can't implement the getter for &str because we cannot return temporary values, we'll have to return String

[lovasoa]

I converted this to a draft PR, feel free to set it back as ready for review when you want me to merge !

[KnorpelSenf]

@lovasoa I think I fixed everything now! From my side, this can be merged. Let's see if CI agrees with me (edit: it does!)

[KnorpelSenf]

@lovasoa please take another look, I completely got rid of as_inner_mut and thereby avoided the std::mem::take. Is there anything obvious that I'm missing as to why this approach won't work?

[lovasoa]

Looks good to me !