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

Adjusting mps writer to the correct structure regarding integer variables declaration #2946

Merged
merged 21 commits into from
Nov 9, 2023

Conversation

Raphael-D-F-Gomes
Copy link
Contributor

@Raphael-D-F-Gomes Raphael-D-F-Gomes commented Aug 11, 2023

Fixes #1132

This pull request aims to fix the bug described in issue #1132. It involves making simple changes to the MPS writer structure to include the separation of discrete variables following the MPS format (https://lpsolve.sourceforge.net/5.0/mps-format.htm). It wasn't added tests to the bug fix, due to the lack of a mps reader function in the library.

Summary/Motivation:

I use the Pyomo library to write mixed-integer linear problems (MILP) to MPS files in my work. I noticed there was a problem in the model, as some binary variables were resulting in floating-point numbers. After researching the Pyomo MPS writer structure, I realized that the MPS file wasn't being exported correctly. So, I expanded the 'ProblemWriter_mps' class and adjusted it to the correct format. These changes worked for me, and I believe they could be a valuable contribution to the library. I created an example using PuLP to read the MPS file exported by Pyomo.

Consider the MILP problem:

from pyomo.environ import *
from pyomo.opt.base import ProblemFormat

from pulp import LpProblem

model = ConcreteModel("Example-mix-integer-linear-problem")

model.x1 = Var(within=NonNegativeIntegers)  # Integer variable
model.x2 = Var(within=NonNegativeReals)      # Continuous variable

model.obj = Objective(expr=3 * model.x1 + 2 * model.x2, sense=minimize)

model.const1 = Constraint(expr=4 * model.x1 + 3 * model.x2 >= 10)
model.const2 = Constraint(expr=model.x1 + 2 * model.x2 <= 7)

mps_path = 'example_milp.mps'
model.write(mps_path, format=ProblemFormat.mps)

_, prob = LpProblem.fromMPS(mps_path)
print(str(prob))

With the corrent version of pyomo, we have the linear problem:

Example-mix-integer-linear-problem:
MINIMIZE
3.0*x1 + 2.0*x2 + 0
SUBJECT TO
c_l_x4_: 4 x1 + 3 x2 >= 10

c_u_x5_: x1 + 2 x2 <= 7

VARIABLES
x1 Continuous
x2 Continuous

Note that the variable "x1" was suppose to be a integer. Now, that is the linear problem with the changes.

Example-mix-integer-linear-problem:
MINIMIZE
3.0*x1 + 2.0*x2 + 0
SUBJECT TO
c_l_x4_: 4 x1 + 3 x2 >= 10

c_u_x5_: x1 + 2 x2 <= 7

VARIABLES
0 <= x1 Integer
x2 Continuous

Changes proposed in this PR:

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.

@mrmundt
Copy link
Contributor

mrmundt commented Aug 11, 2023

Hi @Raphael-D-F-Gomes - Please read the Contributing guide to see how to resolve the linting failure.

@Raphael-D-F-Gomes Raphael-D-F-Gomes changed the title Adjusting mps writer to the correct structure regarding binary variables declaration Adjusting mps writer to the correct structure regarding integer variables declaration Aug 11, 2023
@codecov
Copy link

codecov bot commented Aug 14, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (670d8a1) 87.93% compared to head (410e557) 87.93%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2946      +/-   ##
==========================================
- Coverage   87.93%   87.93%   -0.01%     
==========================================
  Files         769      769              
  Lines       89855    89871      +16     
==========================================
+ Hits        79018    79030      +12     
- Misses      10837    10841       +4     
Flag Coverage Δ
linux 85.24% <100.00%> (-0.01%) ⬇️
osx 75.04% <100.00%> (+<0.01%) ⬆️
other 85.42% <100.00%> (+<0.01%) ⬆️
win 82.48% <100.00%> (+<0.01%) ⬆️

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

Files Coverage Δ
pyomo/core/base/block.py 92.40% <100.00%> (+<0.01%) ⬆️
pyomo/repn/plugins/mps.py 93.33% <100.00%> (+0.30%) ⬆️

... and 1 file with indirect coverage changes

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

Copy link
Contributor

@mrmundt mrmundt left a comment

Choose a reason for hiding this comment

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

I will ask you to write a small test for this. You can do a small baseline comparison test to ensure that behavior is correct.

@Raphael-D-F-Gomes
Copy link
Contributor Author

I will ask you to write a small test for this. You can do a small baseline comparison test to ensure that behavior is correct.

I conducted research on MILP problems encoded in MPS files, but I struggled to locate suitable examples for creating tests that effectively validate the written MPS files. To rigorously assess this feature, I believe it's prudent to employ an alternative library for reading MPS files and verifying the linear problems.

Would it be appropriate to utilize the "pulp" library for testing this particular feature?

@blnicho blnicho self-requested a review September 19, 2023 18:50
Copy link
Member

@jsiirola jsiirola left a comment

Choose a reason for hiding this comment

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

There are a number of changes that I think should be made.

In addition, which solver are you testing against? Or, more specifically, which solver requires the INTORG/INTEND markers? Looking into several solvers' documentation (Cplex, Mosek), they indicate that the markers are not necessary.

pyomo/repn/plugins/mps.py Outdated Show resolved Hide resolved
pyomo/repn/plugins/mps.py Outdated Show resolved Hide resolved
pyomo/repn/plugins/mps.py Outdated Show resolved Hide resolved
pyomo/repn/plugins/mps.py Outdated Show resolved Hide resolved
… marker script // changing set_integer name to in_integer_section // adding marker to the end of integer section
@Raphael-D-F-Gomes
Copy link
Contributor Author

There are a number of changes that I think should be made.

In addition, which solver are you testing against? Or, more specifically, which solver requires the INTORG/INTEND markers? Looking into several solvers' documentation (Cplex, Mosek), they indicate that the markers are not necessary.

The requested modifications have been duly assessed and subsequently implemented.

As outlined in the associated document, there exist two distinct methods for specifying integer variables within the MPS format: one involving markers (INTORG, INTEND) in the COLUMNS section, and the other employing types (BV, LI, SC, UI) in the BOUNDS section. It is noteworthy that the present iteration of Pyomo exclusively supports the latter method for declaring these variables. However, it is worth mentioning that certain solvers, such as lpsolve and the PuLP library, acknowledge the former approach when parsing MPS files.

image

In light of enhancing the robustness of the Pyomo library, it is proposed to introduce the capability to include integer markers within the Pyomo MPS writer. Consequently, a flag has been introduced within the WriterFactory, enabling users to determine whether or not to include integer markers during the writing process.

@Raphael-D-F-Gomes
Copy link
Contributor Author

The broken tests are passing through in my computer, also, it seems that they aren't related to the implementation.

image

image

image

image

@mrmundt mrmundt self-requested a review November 7, 2023 19:43
Copy link
Contributor

@mrmundt mrmundt left a comment

Choose a reason for hiding this comment

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

Pending review from @blnicho or @jsiirola , I think this is good to go.

@blnicho blnicho dismissed jsiirola’s stale review November 9, 2023 19:29

All comments from this review have been addressed

@blnicho blnicho merged commit 71787b7 into Pyomo:main Nov 9, 2023
27 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.

mps writer does not mark discrete variables
6 participants