rust-or / good_lp

Linear Programming for Rust, with a user-friendly API. This crate allows modeling LP problems, and lets you solve them with various solvers.
https://crates.io/crates/good_lp
MIT License
216 stars 35 forks source link

Set coin_cbc verbosity #12

Closed Rahix closed 2 years ago

Rahix commented 2 years ago

Hi,

I was wondering how I can set the verbosity level of coin_cbc through this crate. I found https://github.com/KardinalAI/coin_cbc/issues/3 which looks to be what I want. I also saw that I can get an &Model from CoinCbcProblem::as_inner(). The problem is that Model::set_parameter() requires an &mut reference while I can only obtain an immutable one.

Is there a different solution which I missed? If not, maybe a second method could be added to CoinCbcProblem for getting a mutable reference? If this could prove hazardous for upkeeping internal invariants, making this method unsafe would seem like a good compromise to me.

lovasoa commented 2 years ago

I think a good solution would be to depend on the almost universal log crate, and set the cbc verbosity based on the standard rust log level. What do you think ?

TeXitoi commented 2 years ago

I think exposing parameters is a good idea: there is log, bit also solver tunning available.

The log crate has a complicated syntax, and levels that doesn't really match with coin.

lovasoa commented 2 years ago

We don't want people changing the solver parameters, and for instance removing variables from under our feet, do we ?

How is the log crate syntax complicated?

Rahix commented 2 years ago

I would prefer having direct access to the parameter here. I feel like wiring this up to log is be not a good idea:

  1. log provides a mechanism for libraries and applications to write log messages and an implementation of a filtering mechanism to only show some of those messages. Especially, behind log you'll always need a backend which actually takes care of logging the generated messages somewhere and initializing the filters. CBC doesn't care about all this and just writes its messages to stdout directly. So it wouldn't really tie into the log ecosystem at all. The only "connection" which could be made is by asking log of the (default!) log-level and setting something similar in CBC.
  2. The log-levels from log don't really map to the verbosity levels from CBC well, I think. So it would be confusing for downstream users to understand what log-level from log corresponds to which setting from CBC.
  3. There might also be a mismatch between what the user wants to see logged from the application and what they want to see logged from CBC. To some degree, applications might want to control this, too. You could use a magic module name for setting the CBC log-level but that feels like a hack to me.

In the end I think this wouldn't gain us much and just make the original task I wanted to accomplish more complicated. I think what I wanted to do is what most people will care about: silencing CBC to keep the console free for actually interesting messages.

We don't want people changing the solver parameters, and for instance removing variables from under our feet, do we ?

That's why I would suggest making the access to the CBC model "unsafe":

unsafe fn as_inner_mut(&mut self) -> &mut Model {
    // ...
}

This way people can't possibly destroy invariants assumed by good_lp without unsafe code.

In parallel to adding the above (which is probably useful beyond this issue alone), for logging in particular, maybe adding another method specifically to set the verbosity might be a good idea? It would certainly help people searching through good_lp documentation for terms like "log" or "verbosity" to find what they need... For example:

fn set_solver_verbosity(&mut self, v: &str) {
    // ...
}
TeXitoi commented 2 years ago

You can also add a fn set_parameter(key: &str, value: &str) without any invariant breaking.

ihowell commented 2 years ago

I would like to add that we are experiencing the same problems in the highs solver. Any way of changing the verbosity would be extremely appreciated. I think being able to set the options on the models themselves would be extremely useful.