pyomo icon indicating copy to clipboard operation
pyomo copied to clipboard

New add_column() method for appsi solvers

Open darrylmelander opened this issue 2 years ago • 7 comments

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.

darrylmelander avatar Apr 12 '23 22:04 darrylmelander

@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 avatar Apr 12 '23 22:04 darrylmelander

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 :warning:
pyomo/contrib/appsi/solvers/gurobi.py 85.71% 2 Missing :warning:
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%) :arrow_up:
osx 75.12% <76.38%> (+<0.01%) :arrow_up:
other 85.50% <94.44%> (+<0.01%) :arrow_up:
win 82.57% <94.44%> (+0.01%) :arrow_up:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Apr 17 '23 14:04 codecov[bot]

@michaelbynum, can you take a look at this?

jsiirola avatar May 30 '23 18:05 jsiirola

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 avatar Dec 19 '23 19:12 michaelbynum

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

blnicho avatar Aug 06 '24 21:08 blnicho

@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 avatar Aug 14 '24 17:08 bknueven

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? https://github.com/Pyomo/pyomo/blob/b5825e9e146552690d942903386102105251464c/pyomo/solvers/plugins/solvers/persistent_solver.py#L197-L214

bknueven avatar Aug 14 '24 17:08 bknueven