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

Add method for adding piecewise linear constraints #776

Merged
merged 15 commits into from
Apr 1, 2024

Conversation

Joao-Dionisio
Copy link
Collaborator

Someone requested a method for adding piecewise linear constraints. I just converted the relevant finished example to scip.pxi. Using a draft because there are more efficient ways of doing this (both algorithmically but also by using more C functions).

Copy link

codecov bot commented Jan 11, 2024

Codecov Report

Attention: Patch coverage is 5.55556% with 17 lines in your changes are missing coverage. Please review.

Project coverage is 52.49%. Comparing base (cb7e441) to head (3809c61).

Files Patch % Lines
src/pyscipopt/recipes/piecewise.py 0.00% 17 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #776      +/-   ##
==========================================
- Coverage   52.72%   52.49%   -0.24%     
==========================================
  Files          17       18       +1     
  Lines        3831     3848      +17     
==========================================
  Hits         2020     2020              
- Misses       1811     1828      +17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Opt-Mucca
Copy link
Collaborator

@Joao-Dionisio I'm not sure on this change. I'll put some reasons as to why.

  • There are alternative formulations for modelling piecewise-linear functions. This function implies that we have done research onto the bets performing formulation.
  • I don't think it is the responsibility of this package to decide on the formulations. We should model whatever the user gives us, but ultimately it should depend on the user to define this. (I guess this is slowly changing as similar packages become more user friendly and easier to access)
  • The function as is currently only supports 2-D functions, which is only a subset of all piecewise-linear formulations.

Also as a small question (haven't yet written everything down to check fully): Is the SOS1 constraint necessary? Or would a sum of the binary variables suffice?

Regardless I think something like this should be made into an example or a test. I just disagree with making it a foreard facing functionality for users.

@Joao-Dionisio
Copy link
Collaborator Author

Yeah, I understand your reservations, but I think having the possibility of doing something like this does not prevent the user from using their own formulations. Also, if I'm not an expert and just want to use SCIP to solve a simple industry problem, having to go through all the trouble of defining a piecewise linear constraint can be super annoying, especially considering the many ways of doing it.

I agree that before an eventual merge, some more research into something more efficient than the direct way of doing this is necessary, and maybe we should add a disclaimer to the documentation saying that the method is precisely to facilitate modeling, it's not supposed to be the best way of solving the user's problem.

Regarding the SOS1, you're right, the sum would suffice. I just figured that SCIP's SOS1 constraints would somehow be more efficient, but I have no clue.

Your final comment about turning this into an example is interesting because what I did was just grabbing this from the examples :)

But yeah, in the end, it does come down to how user-friendly we want PySCIPOpt to be, and you guys should have the final word on this, of course.

Copy link
Member

@mmghannam mmghannam left a comment

Choose a reason for hiding this comment

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

Thanks a lot @Joao-Dionisio! I'm all for convenience functions. I will respond to @Opt-Mucca's reservations in another comment.

src/pyscipopt/scip.pxi Outdated Show resolved Hide resolved
src/pyscipopt/scip.pxi Outdated Show resolved Hide resolved
src/pyscipopt/scip.pxi Outdated Show resolved Hide resolved
src/pyscipopt/scip.pxi Outdated Show resolved Hide resolved
src/pyscipopt/scip.pxi Outdated Show resolved Hide resolved
@mmghannam
Copy link
Member

Hey @Opt-Mucca 😄
As João said, this makes it much easier for someone to write a 2d piecewise linear function without knowing much about it. But you have some legitimate concerns but they can be addressed I believe.

  • There are alternative formulations for modelling piecewise-linear functions. This function implies that we have done research onto the bets performing formulation.

We can solve this by adding a disclaimer or clearly defining where the formulation comes from.

  • I don't think it is the responsibility of this package to decide on the formulations. We should model whatever the user gives us, but ultimately it should depend on the user to define this. (I guess this is slowly changing as similar packages become more user friendly and easier to access)

It does not decide on it. Maybe even in the future when there are multiple formulations implemented another parameter formulation could be passed to this method to choose which.

  • The function as is currently only supports 2-D functions, which is only a subset of all piecewise-linear formulations.

This can be mentioned in the docstring of the method and could be improved in a next iteration without changing the API I believe.

Regardless I think something like this should be made into an example or a test. I just disagree with making it a foreard facing functionality for users.

This ultimately saves some time for users who are new to the topic. I think what we should guarantee is that the function is correct and that the documentation is clear and try to make it more general. So with the changes I mentioned above, what do you think?

@Opt-Mucca
Copy link
Collaborator

@Joao-Dionisio for the SOS1 constraint you should interpret it as the following: At most one entry in the constraint can be non-zero.
In the case of an array of binary variables this is equivalent to 0 <= z1 + .... + zN <= 1.
From my understanding this isn't what you want though, and you need exactly one of the binary variables to be active. So you'd need the constraint z1 + ..... + zN == 1.
In general for SOS1 constraints: These should be used if they can save the user writing out a huge amount of additional constraints and introducing many additional variables. The most common scenario is when they can replace big-M constraints. One should be careful though, because these constraints aren't handled like the more traditional constraints. Coming up with a linear representation isn't really possible, and there are then clear downsides for performance as your relaxation gets further away from your actual problem. The usual result is that way more branching is needed (but now your relaxations are much smaller and tidier than they'd have been otherwise), and that a lot of heuristics will now fail to produce feasible solutions for the original problem.

@Opt-Mucca
Copy link
Collaborator

@mmghannam Hallo! So I do feel that this automatic formulation is a current trend, and is probably very helpful to users. It just starts blurring the line between what packages like these are traditionally supposed to offer.

Before adding such a function I would suggest making thorough tests to ensure the formulation is correct. Functionality like this now allows a new type of error to exist, in that it's not a communication issue with SCIP or a simple SCIP error, but an actual incorrect model (of course this has existed before, but it was very easy to locate and fix because the functions are mostly just wrappers). I would also be more for making the function more general than just 2-D. Could we not just accept and a list of lists and pretend it's a matrix and then model arbitrary dimensions?

@Joao-Dionisio
Copy link
Collaborator Author

Thanks for the explanation on SOS1 constraints, @Opt-Mucca! I had never really looked into them.
And thank you for taking care of the issues this morning :)

Agreed on the generalization and on the more thorough testing.

@mmghannam mmghannam marked this pull request as ready for review April 1, 2024 08:39
@mmghannam mmghannam merged commit d1a9895 into master Apr 1, 2024
1 of 3 checks passed
@Joao-Dionisio Joao-Dionisio deleted the piecewise-linear branch April 3, 2024 09:07
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.

None yet

3 participants