Skip to content
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

feat: add support for setting initial solutions with cbc #71

Merged
merged 14 commits into from
Dec 30, 2024

Conversation

KnorpelSenf
Copy link
Contributor

@KnorpelSenf KnorpelSenf commented Dec 13, 2024

This is a first attempt at adding support for initial solutions. For now, only coin_cbc is supported. Compatibility with more solvers can be added in subsequent pull requests.

For some reason, the added test still fails. It's probably a dumb oversight, but until somebody spots it, I would be happy to receive feedback on whether the API and implementation of this feature go in the right direction. If you have complaints about the naming (should we rename set_initial_solution to initial_solution or initial or initialize or init or set_initial or with_initial_solution?), please let me know and I'll change it.

Towards #55.

src/solvers/mod.rs Outdated Show resolved Hide resolved
Co-authored-by: Ophir LOJKINE <contact@ophir.dev>
@lovasoa
Copy link
Collaborator

lovasoa commented Dec 13, 2024

The design seems good to me

@KnorpelSenf
Copy link
Contributor Author

Awesome! Now I just have to find out why the problem in the test has no solution the second time I compute it

@KnorpelSenf KnorpelSenf marked this pull request as ready for review December 13, 2024 11:21
@KnorpelSenf
Copy link
Contributor Author

I just simplified the test a lot and that fixed it. I am not sure if there is still something wrong now?

@lovasoa do you think we now need a more ergonomic way to construct CoinCbcSolutions? The current changes basically only make it easy to solve a problem once and then recreate it. But what if somebody already has an initial solution without having solved the problem via this library?

@TeXitoi
Copy link
Contributor

TeXitoi commented Dec 13, 2024

But what if somebody already has an initial solution without having solved the problem via this library?

It’s an important case I think. In my usecase, I have a solution that I know is feasible and good quality.

@KnorpelSenf
Copy link
Contributor Author

But what if somebody already has an initial solution without having solved the problem via this library?

It’s an important case I think. In my usecase, I have a solution that I know is feasible and good quality.

The same is true for me. The question remains if we need a more ergonomic API to construct a solution. Maybe not, because we're going to have a way to give an initial value to each variable? I'm undecided on this so far

@lovasoa
Copy link
Collaborator

lovasoa commented Dec 15, 2024

Yes, I think we need a way to build solutions for this to be helpful. Maybe implement FromIterator<(Variable, Value)>

@KnorpelSenf
Copy link
Contributor Author

@lovasoa do you want this done in this PR? If no, what else is needed before this can be merged?

@lovasoa
Copy link
Collaborator

lovasoa commented Dec 19, 2024

Yes, I think the feature is not very useful without this

src/solvers/coin_cbc.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@lovasoa lovasoa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great ! I think we should accept an iterator instead of a vec, and then we are good to merge 👍

src/solvers/mod.rs Outdated Show resolved Hide resolved
src/solvers/coin_cbc.rs Outdated Show resolved Hide resolved
@KnorpelSenf
Copy link
Contributor Author

Nice, will do!

I can imagine that setting each variable individually is not as good as setting the entire solution at once if one has a full solution available. Should we still have a way to set complete initial solutions?

I honestly don't really see how that would be useful because I don't see a way to construct these solutions, but I just wanted to make sure we're on the same page with this:)

@lovasoa
Copy link
Collaborator

lovasoa commented Dec 29, 2024

The current implementation looks good to me. I'll merge it as soon as you make the fix.

@TeXitoi
Copy link
Contributor

TeXitoi commented Dec 29, 2024

If we could modify a model, it could be useful to use the solution. An example is lexicographic objectives:

  • you create the problem with the first objective
  • you set a constraint "the previous objective must be at least the quality of the solution found"
  • you set the next objective
  • facultative, but can improve performance a lot: you set the initial solution to the previous found solution
  • you continue until all objectives are optimized.

But, as far as I remember, the current interface doesn’t allow this.

@KnorpelSenf
Copy link
Contributor Author

I agree. I would also like to be able to modify problems and then reuse previous solutions from previous runs. But this is out of scope for the current PR. I might look into this in the future.

@lovasoa lovasoa merged commit 1227b3e into rust-or:main Dec 30, 2024
1 check passed
@KnorpelSenf KnorpelSenf deleted the full-initial-solutions branch December 30, 2024 10:57
@lovasoa
Copy link
Collaborator

lovasoa commented Dec 30, 2024

🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants