Add branching priorities to CPLEXSHELL #1

ruaridhw · 2020-02-20T11:33:38Z

Fixes Pyomo#1299

Summary/Motivation:

To be able to use the branching priority capability of commercial solvers when solving MIP problems

Changes proposed in this PR:

Include branch_priority and branch_direction attributes of variable data
Read off these attributes as part of the CPLEXSHELL solve and write them to a CPLEX .ord file
Update the CPLEX OPL script to read these .ord file should it exist

Legal Acknowledgement

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

I agree my contributions are submitted under the BSD license.
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.

- Include `branch_priority` and `branch_direction` attributes of variable data - Read off these attributes as part of the CPLEXSHELL solve and write them to a CPLEX .ord file - Update the CPLEX OPL script to read these .ord file should it exist

- End-to-end tests of `opt.solve()` using branching priorities with CPLEXSHELL

- Declare `priority` and `direction` suffixes on the model instead of attributes on the variable data in order to represent branching decisions

- Looks like there's some circular imports failing specifically for Py2.7 - Follow the same (hacky) local import design chosen in 0935e75

mhusbyn

See comments. Main bits:

Pulling out direction class into a more global enum and making the translation CPLEX specific
Splitting up _write_priorities_file (if it makes sense)

mhusbyn · 2020-02-20T16:58:08Z

pyomo/solvers/plugins/solvers/CPLEX.py

+class CPLEXBranchDirection:
+    default = 0
+    down = -1
+    up = 1
+


This could probably be pulled out to be a 'global' enum so it can be used for all solvers that support branching priorities. The only solver specific stuff here is the translation so that can be done in each individual solver.

That way you can build your model agnostic of what solver it will run on, which I don't think would be possible with the current implementation.

I see you have to specify a data type when declaring the Suffix. I guess you should add a 'data_type' attribute or something as well in that case that can be used.

The only solver specific stuff here is the translation

Ah not quite 😉 These magic integer values themselves are CPLEX-specific. See here. Having said that, it turns out that Gurobi also just happens to share the same magic values... So I'll do the refactor!

Where are these magic integer values being used for CLEX specific stuff? Aren't they being translated in to_str?

As discussed, they will be used in the CPLEXDirect and GurobiDirect solver implementations which have not been included here. For now, I've removed CPLEXBranchDirection and moved the translation to ORDFileSchema

mhusbyn · 2020-02-20T17:00:52Z

pyomo/solvers/plugins/solvers/CPLEX.py

+        try:
+            return {CPLEXBranchDirection.down: "DN", CPLEXBranchDirection.up: "UP"}[
+                branch_direction
+            ]
+        except KeyError:
+            return ""


No idea if more efficient/readable but dict.get is an option here. But Python tends to favour 'easier to ask for forgiveness' over 'look before you leap' so I guess this is fine too.

But Python tends to favour 'easier to ask for forgiveness'

Yeah, that was my intention

mhusbyn · 2020-02-20T17:06:54Z

pyomo/solvers/plugins/solvers/CPLEX.py

+
+    def _write_priorities_file(self, instance) -> None:
+        """ Write a variable priorities file in the CPLEX ORD format. """
+        from pyomo.core.base import Var


Is there a reason this is an inline import?

I was copying _warm_start() where this is a local import too... I'll try removing both and 🤞 nothing breaks

mhusbyn · 2020-02-20T17:09:34Z

pyomo/solvers/plugins/solvers/CPLEX.py

+    SUFFIX_PRIORITY_NAME = "priority"
+    SUFFIX_DIRECTION_NAME = "direction"
+
+    def _write_priorities_file(self, instance) -> None:


NB: This has a return type (I'm assuming not Python 2.7 compliant)

Ah perfect, that's probably the reason 2.7 is breaking. Thanks!

mhusbyn · 2020-02-20T17:10:02Z

pyomo/solvers/plugins/solvers/CPLEX.py

+    SUFFIX_PRIORITY_NAME = "priority"
+    SUFFIX_DIRECTION_NAME = "direction"
+
+    def _write_priorities_file(self, instance) -> None:


What is instance? Is it worth documenting or is it Pyomo lingo?

Yup, it's standard (see _warm_start())

mhusbyn · 2020-02-20T17:23:22Z

pyomo/solvers/plugins/solvers/CPLEX.py

+                        ORDFileSchema.ROW(byObject[id(child_var)], priority, child_var_direction)
+                    )
+
+            ord_file.write(ORDFileSchema.FOOTER)


Worth splitting this method up a bit? To me it seems like there's 2-3 steps here:

input preparation

translation

writing to the file

It looks to me like some of this logic is not CPLEX specific so if we manage to split this method up into parts it'll most likely be easier to implement extra solvers later.

This is based on my assumption that it is mainly the translation that is solver specific.

mhusbyn · 2020-02-21T11:09:08Z

pyomo/solvers/tests/checks/test_cplex.py

+            ' UP x11 2\n'
+            'ENDATA\n'
+        )
+


For these tests, please use the priority constant instead of magic numbers. That'll make them easier to maintain, e.g. if we decide to represent them with different numbers or something.

And there is no ambiguity of what they mean.

Updated, however I felt this was a bit too convoluted for test_use_variable_priorities()

mhusbyn · 2020-02-21T11:15:50Z

pyomo/solvers/tests/checks/test_cplex.py

+        assert priorities_file == (
+            '* ENCODING=ISO-8859-1\n'
+            'NAME             Priority Order\n'
+            '  x1 1\n'
+            ' DN x2 2\n'
+            ' DN x3 2\n'
+            ' DN x4 2\n'
+            ' DN x5 2\n'
+            ' DN x6 2\n'
+            ' DN x7 2\n'
+            ' DN x8 2\n'
+            ' DN x9 2\n'
+            ' DN x10 2\n'
+            ' UP x11 2\n'
+            'ENDATA\n'
+        )


Perhaps this assertion is neater if you read the contents of priorities_file_name before doing the final solve and comparing that against priorities_file? They should be the same right?

- Update tests to avoid usage of "magic numbers" and reference `BranchDirection` where possible

Remove Port.Conservative, be more selective for evar bounds

merge master into pynumero_mumps

Minor formatting changes to consolidate unix & mpi drivers

Extend sorted_robust API

Cleaning up sensitivity_toolbox tests

Further minor polishing fixes

ruaridhw changed the title ~~🔨 Add branching priorities to CPLEXSHELL~~ Add branching priorities to CPLEXSHELL Feb 20, 2020

ruaridhw added 3 commits February 20, 2020 14:17

🚨 Add tests of solving with priorities

81f0a6e

- End-to-end tests of `opt.solve()` using branching priorities with CPLEXSHELL

🔨 Use Suffixes instead of _GeneralVarData

5e44956

- Declare `priority` and `direction` suffixes on the model instead of attributes on the variable data in order to represent branching decisions

ruaridhw force-pushed the cplex_branching_priorities branch from 9bbba62 to 5e44956 Compare February 20, 2020 15:24

ruaridhw added 5 commits February 20, 2020 15:32

🔨 Allow indexed variables to have separate directions

c4b9c53

🐛 Handle both non-indexed vars in Suffix

b468b2b

🚨 Update tests to work when CPLEX is unavailable

495776d

🐛 Fix test

dbccdf2

🐛 Fix imports

ba38d43

- Looks like there's some circular imports failing specifically for Py2.7 - Follow the same (hacky) local import design chosen in 0935e75

mhusbyn suggested changes Feb 21, 2020

View reviewed changes

🔨 Make BranchDirection global and simplify _write_priorities_file()

e4f43f0

- Update tests to avoid usage of "magic numbers" and reference `BranchDirection` where possible

ruaridhw requested a review from mhusbyn February 21, 2020 12:16

🔨 Remove unnecessary CPLEXBranchDirection

ae59e09

mhusbyn approved these changes Feb 21, 2020

View reviewed changes

mhusbyn merged commit 97baa7a into master Feb 21, 2020

ruaridhw pushed a commit that referenced this pull request Mar 4, 2020

Merge pull request #1 from jsiirola/conservative-rule

3cea644

Remove Port.Conservative, be more selective for evar bounds

begunefeoglu pushed a commit to begunefeoglu/pyomo that referenced this pull request Apr 1, 2020

Merge pull request flexciton#1 from Pyomo/master

e0cfc5c

merge master into pynumero_mumps

begunefeoglu pushed a commit to begunefeoglu/pyomo that referenced this pull request Apr 1, 2020

Merge pull request flexciton#1 from jsiirola/unix_coverage

eaa8492

Minor formatting changes to consolidate unix & mpi drivers

ruaridhw pushed a commit that referenced this pull request May 24, 2021

Merge pull request #1 from jsiirola/sorted-robust-write

45632e6

Extend sorted_robust API

Tomasz-Kluczkowski pushed a commit that referenced this pull request Sep 16, 2021

Merge pull request #1 from blnicho/kaug-sens

ee7a76c

Cleaning up sensitivity_toolbox tests

Tomasz-Kluczkowski pushed a commit that referenced this pull request Sep 16, 2021

Merge pull request #1 from chrysg/pyros_pr

180e891

Further minor polishing fixes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add branching priorities to CPLEXSHELL #1

Add branching priorities to CPLEXSHELL #1

ruaridhw commented Feb 20, 2020

mhusbyn left a comment

mhusbyn Feb 20, 2020

mhusbyn Feb 21, 2020

ruaridhw Feb 21, 2020 •

edited

Loading

mhusbyn Feb 21, 2020

ruaridhw Feb 21, 2020

mhusbyn Feb 20, 2020

ruaridhw Feb 21, 2020

mhusbyn Feb 20, 2020

ruaridhw Feb 21, 2020

mhusbyn Feb 20, 2020

ruaridhw Feb 21, 2020

mhusbyn Feb 20, 2020

ruaridhw Feb 21, 2020

mhusbyn Feb 20, 2020

mhusbyn Feb 21, 2020

ruaridhw Feb 21, 2020

mhusbyn Feb 21, 2020

ruaridhw Feb 21, 2020

Add branching priorities to CPLEXSHELL #1

Add branching priorities to CPLEXSHELL #1

Conversation

ruaridhw commented Feb 20, 2020

Fixes Pyomo#1299

Summary/Motivation:

Changes proposed in this PR:

Legal Acknowledgement

mhusbyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruaridhw Feb 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruaridhw Feb 21, 2020 •

edited

Loading