rust-or / rust-lp-modeler

Lp modeler written in Rust
MIT License
96 stars 29 forks source link

Allow setting threads and seconds for CBC #29

Closed zappolowski closed 5 years ago

zappolowski commented 5 years ago

I needed a way to speed up calculations and also restrict the total runtime of the optimization for the CBC solver. I've opted for a generic approach (first used specific fields in LpProblem, but this approach will create a lot of boilerplate in the long run) as this seems to be easily extendable for other options.

I also took the opportunity to refactor SolverTrait::run for CbcSolver to use ? instead of nested match (I can revert these changes if they're undesired).

jcavat commented 5 years ago

Thank you, I will take a look soon

jcavat commented 5 years ago

Hello, thanks for your PR ! I would prefer to have something more typesafe in case we apply multiple times the same function. What about an Option<u32> threads and Option<u32> max_seconds into the struct ? Moreover, do you think it will be useful to use f32 for seconds instead of u32 ?

zappolowski commented 5 years ago

I would prefer to have something more typesafe in case we apply multiple times the same function.

I'd say, that type safety is ensured by the interface of the setters (e.g. threads just allows passing u32 values) already and the internal representation is not available for direct modification. Ultimately, the arguments will be converted to String anyhow as they're passed to Command::args.

What about an Option<u32> threads and Option<u32> max_seconds into the struct ?

That's what I tried first, but then the construction of additional_args becomes repetitive:

let mut additional_args = Vec::new();

if let Some(threads) = self.params.threads {
    additional_args.push("threads".to_owned());
    additional_args.push(threads.to_string());
};

if let Some(seconds) = self.params.seconds {
    additional_args.push("seconds".to_owned());
    additional_args.push(seconds.to_string());
};

(maybe this could be moved to a function, though).

Additionally, having separate fields on LpProblem itself would require to adjust all other setters to take care of the new field. Also, converting the parameter name to string is moved away from where it is set, which might not be that easy to understand in the long run.

Some advantage of my approach is, that it's easier to extend. E.g. adding an option to set the allowableGap would be just

pub fn allowable_gap(&self, allowable_gap: f32) -> CbcSolver {
    self.add_param("allowableGap".to_owned(), allowable_gap.to_string())
}

Another approach could be to create another struct of parameters. But this will be - more or less - a big key-value struct. That's why I settled for a HashMap finally.

Moreover, do you think it will be useful to use f32 for seconds instead of u32 ?

My thinking was to stick to what CBC accepts at this point and seconds are listed with double (in this regard, I should use f64 to be fully correct). But I don't have strong feelings towards any of the possibilities.

jcavat commented 5 years ago

I changed some parts to force solvers respecting traits for specific configuration. Commits involved : d3949f1f, c506bf6