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

New add_column() method for appsi solvers #2804

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

darrylmelander
Copy link
Contributor

Summary/Motivation:

This PR adds a new add_column() method to the solvers in the pyomo.contrib.appsi.solvers package. Like the add_column() method in the persistent solvers in the main pyomo.solvers package, this function allows you to add a new variable, and add it to the objective and to existing constraints, all in a single function call. This gives persistent solvers an opportunity to add the new variable more efficiently than is possible when modifying existing constraints one by one.

Changes proposed in this PR:

  • Add a new add_column() method to appsi solvers.

Legal Acknowledgement

By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the BSD license.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

This method adds a variable along with its coefficients in the
objective and in constraints, all in a single call. Provides the
opportunity for more efficient changes to an existing model.
@darrylmelander
Copy link
Contributor Author

@michaelbynum, This is a PR you will probably want to review.

One thing I'd like to note is that I had to comment out some lines in the test for this PR (see these commented out lines). Those lines were intended to verify that the pyomo model components were modified as expected, but I could never get the comparisons to consistently work as expected, even though the string representation of the expressions showed they were equivalent. If there's a preferred way to make those comparisons, great, I'd love to have them added to the test.

@darrylmelander darrylmelander marked this pull request as ready for review April 12, 2023 22:15
@codecov
Copy link

codecov bot commented Apr 17, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (ae85c0c) 87.98% compared to head (e9797f9) 87.99%.
Report is 158 commits behind head on main.

Files Patch % Lines
pyomo/contrib/appsi/base.py 95.00% 2 Missing ⚠️
pyomo/contrib/appsi/solvers/gurobi.py 85.71% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2804   +/-   ##
=======================================
  Coverage   87.98%   87.99%           
=======================================
  Files         770      770           
  Lines       90242    90310   +68     
=======================================
+ Hits        79400    79465   +65     
- Misses      10842    10845    +3     
Flag Coverage Δ
linux 85.32% <94.44%> (+<0.01%) ⬆️
osx 75.12% <76.38%> (+<0.01%) ⬆️
other 85.50% <94.44%> (+<0.01%) ⬆️
win 82.57% <94.44%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@jsiirola
Copy link
Member

@michaelbynum, can you take a look at this?

Copy link
Contributor

@michaelbynum michaelbynum left a comment

Choose a reason for hiding this comment

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

A couple minor comments below.

# Add the variable to the objective
if obj_coef != 0.0 and self._objective is not None:
# Add variable to the pyomo objective
self._objective.expr = self._objective.expr + obj_coef * var
Copy link
Contributor

Choose a reason for hiding this comment

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

This is certainly convenient, but I'm not sure the solver interface should ever modify the pyomo model...

Copy link
Contributor

Choose a reason for hiding this comment

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

Having said that, I don't think there is a better way to do this. At least the doc string is clear.

Comment on lines +821 to +823
The column will have already been added to the Pyomo model before
this method is called, such that the variable is already incorporated
into the passed in constraints.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct?

Copy link
Contributor

@michaelbynum michaelbynum left a comment

Choose a reason for hiding this comment

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

we are pondering the above questions a bit

@michaelbynum
Copy link
Contributor

I think we should move this method into a "helper function" that takes in a model and a solver and modifies both. This way, we maintain that solver interfaces do not modify models as a "side effect".

@blnicho
Copy link
Member

blnicho commented Aug 6, 2024

@darrylmelander @michaelbynum this PR has been open for a while, any updates or plans for finishing this?

@bknueven
Copy link
Contributor

@darrylmelander @michaelbynum this PR has been open for a while, any updates or plans for finishing this?

If @darrylmelander is unavailable I could pick this up. It would still be a useful thing for APPSI (or any of our "in memory") solver interfaces to have.

@bknueven
Copy link
Contributor

I think we should move this method into a "helper function" that takes in a model and a solver and modifies both. This way, we maintain that solver interfaces do not modify models as a "side effect".

@michaelbynum would the function signature we currently use for persistent solvers be better?

def add_column(self, model, var, obj_coef, constraints, coefficients):
"""Add a column to the solver's and Pyomo model
This will add the Pyomo variable var to the solver's
model, and put the coefficients on the associated
constraints in the solver model. If the obj_coef is
not zero, it will add obj_coef*var to the objective
of both the Pyomo and solver's model.
Parameters
----------
model: pyomo ConcreteModel to which the column will be added
var: Var (scalar Var or single VarData)
obj_coef: float, pyo.Param
constraints: list of scalar Constraints of single ConstraintDatas
coefficients: list of the coefficient to put on var in the associated constraint
"""

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

Successfully merging this pull request may close these issues.

7 participants