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 addExprNonlinear #760

Merged
merged 35 commits into from
Feb 25, 2024
Merged

Add addExprNonlinear #760

merged 35 commits into from
Feb 25, 2024

Conversation

Joao-Dionisio
Copy link
Collaborator

This is not the best way to solve the issue, but I think it gets the work done.

  • SCIPaddExprNonlinear requires a nonlinear constraint, but it should be possible to convert a linear constraint into a nonlinear one in scip.pxi (x = self.addVar(lb=0,ub=0), self.addCons(linear expr + x**2 <= RHS)). This would work with other methods.
  • I'm using a Python union, since GenExpr doesn't include Expr. Most likely it should.
  • I couldn't find a proper way to get a scip expression from the python expression, so I add a temporary constraint, and then delete it.

@Opt-Mucca
Copy link
Collaborator

I feel like I'm playing the opposition on these merge requests......

In what context does something like this come up @Joao-Dionisio ? I'm not against the change, but it feels incredibly niche. Couldn't you simply just remove the constraint and then re add it with the now modified expression? (Adding a linear term to the non-linear expression should work fine)

@Joao-Dionisio
Copy link
Collaborator Author

No worries @Opt-Mucca, I think it was me who spammed PRs of dubious interest :)

This came up with Issue #713, and it seems like it could be helpful, maybe for reoptimizations and such. I suppose it would serve the same purpose that it serves in SCIP.

@Opt-Mucca
Copy link
Collaborator

@Joao-Dionisio I'm convinced for this one after seeing the issue you commented. I still think you could simply keep track of the expression inside of the constraint, but that is actually duplicate work, and maybe not that intuitive.

Suggestion: Do you need to add coef * expr instead of just the expression? The user could always just put coef * expr as the input then.

@Joao-Dionisio
Copy link
Collaborator Author

@Opt-Mucca yeah, you just need the expression, but I tried to keep the function as close to the SCIP function as I could, which does take coef as an input.

@Opt-Mucca
Copy link
Collaborator

Then I'm happy with this one!

@Joao-Dionisio Joao-Dionisio linked an issue Feb 2, 2024 that may be closed by this pull request
@Joao-Dionisio
Copy link
Collaborator Author

Joao-Dionisio commented Feb 2, 2024

Oh man, what have I done... I just wanted to add a TODO :(

Anyway, we talked about accepting this if we added a TODO. Since it seems a bit annoying to actually solve the problems, and they're not that big, then I say we do this. It also frees up an issue.

Copy link

codecov bot commented Feb 2, 2024

Codecov Report

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

Project coverage is 52.76%. Comparing base (5a166be) to head (676939d).

Files Patch % Lines
src/pyscipopt/scip.pxi 75.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #760      +/-   ##
==========================================
+ Coverage   52.69%   52.76%   +0.07%     
==========================================
  Files          17       17              
  Lines        3809     3817       +8     
==========================================
+ Hits         2007     2014       +7     
- Misses       1802     1803       +1     

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

@Joao-Dionisio Joao-Dionisio merged commit 3833085 into master Feb 25, 2024
3 checks passed
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.

Accessing information of constraints
3 participants