-
Notifications
You must be signed in to change notification settings - Fork 526
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
GDP rewrite #354
GDP rewrite #354
Conversation
…variables like in gams
…d troubles), fixes variables in stickies to validate model, changes add slacks transformation to modify the body of the constraint directly
…esults from first layout on minlp.org (and it's rightgit add stickies.pygit add stickies.py)
… am not sure they are right yet.
… begins to add bounds in the batch processing model
…s debugging stuff in add slacks
…ing model, and notes Francisco's suggestion for second term of disjunction for whether or not there is a storage tank (but no conclusion yet)
…essions by digging into the _SumExpression objects
…essions by digging into the _SumExpression objects
…ession tests pass, adding unit test file for bigm
…ew structure of transformed model
…ansformation block and xor constraints added to parent block and using original indicator variables
…tions, indexed constraints in the disjunction)
GDPopt solver initial implementation
Substituting he disaggregated variables into a constraint should preserve the "equality nature" of the constraint. In addition, catch the special case of "x == 0" in a disjunct so we can fix the disaggregated variable to 0.
This supports declaring "empty" disjunctions; fixes #241.
Refactor the gdp.cuttingplane transformation - avoid the use of CUIDs - improved initialization of separation problems - graceful exit when subproblem solves fail
Most of the changes are due to: - equality constraints are no longer split into inequalities - BigM constraints on disaggregated vars that are redundant to bounds are omitted.
return suffix_list | ||
|
||
def _apply_to(self, instance, targets=None, **kwds): | ||
config = self.CONFIG().set_value(kwds.pop('options', {})) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm very confused how this is passing tests, because on my system, set_value()
has no return value (so it's always None
), which causes config = None
, which causes issues when you try to do config.set_value(kwds)
below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need the current pyutilib master.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes. That seems to have done the trick.
CONFIG = ConfigBlock("gdp.bigm") | ||
CONFIG.declare('bigM', ConfigValue( | ||
default=None, | ||
domain=_to_dict, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does domain
mean with respect to a ConfigBlock
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the domain
is passed to the ConfigValue
constructor. domain
is a callable object (could be a type, function, or functor). When a value is set, the incoming value is passed as the single argument to __call__
and the return value is what is stored in the ConfigValue
. At it's simplest, it can be a type like int
so that all incoming values are automatically cast to int
. In this case it is a little more complicated, as it accepts dicts or "values" (int/float/tuple). The former is preserved and the latter is cast to {None: val}
.
To answer your question, ConfigBlocks
can not have domains, although they accept an implicit_domain
, which is used when the ConfigBlock
accepts undeclared (implicit) keys - in which case, any implicit values are passed through the implicit_domain
callable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few questions.
Also, I should note that bigM
doesn't currently support big-E Expression objects attached to disjuncts. I assume that chull does not as well.
I got tired of looking through all the examples.
|
||
# bounds for box coordinates | ||
# TODO: What are they trying to accomplish here? Why is it always index | ||
# 2 for the first two?? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an error in the original GAMS file. The intent is to find the min and max x and y coordinates that the boxes can have, based on the circle locations and radii. The code as written works because circle 2 has the higher x and y coordinate values.
def no_overlap_rule(model, i, j): | ||
return [model.no_overlap_disjunct[i, j, direction] for direction in \ | ||
model.BoxRelations] | ||
model.no_overlap_disjunction = Disjunction(model.BoxPairs, rule=no_overlap_rule) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we going to support the shortcut notation?
@model.Disjunction(model.BoxPairs)
def no_overlap_rule(model, i, j):
...
Similarly with Disjunct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. That notation should (already) be supported.
|
||
model = AbstractModel() | ||
|
||
# TODO: it looks like they set a bigM for each j. Which I need to look up how to do... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
j
's are stages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, j's are stages. I think the gams file uses 3 different Big-M values, one for each disjunction. They are indexed by stage, but I don't think they actually depend on it. I never got as far as making my Big-M values match those ones.
UnitsInPhaseUB = log(6) | ||
UnitsOutOfPhaseUB = log(6) | ||
# TODO: YOU ARE HERE. YOU HAVEN'T ACTUALLY MADE THESE THE BOUNDS YET, NOR HAVE YOU FIGURED OUT WHOSE | ||
# BOUNDS THEY ARE. AND THERE ARE MORE IN GAMS. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems unfinished?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is likely... I ran into some mistakes in the gams file for this problem, and so I know I wasn't ever able to match their results for this one.
model.STAGES = Set(ordered=True) | ||
model.PARALLELUNITS = Set(ordered=True) | ||
|
||
# TODO: this seems like an over-complicated way to accomplish this task... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given what's written, the filter seems reasonable.
############### | ||
|
||
def storage_tank_selection_disjunct_rule(disjunct, selectStorageTank, j): | ||
model = disjunct.model() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this line is not strictly necessary? :0)
return [model.storage_tank_selection_disjunct[selectTank, j] for selectTank in [0,1]] | ||
model.select_storage_tanks = Disjunction(model.STAGESExceptLast, rule=select_storage_tanks_rule) | ||
|
||
# though this is a disjunction in the GAMs model, it is more efficiently formulated this way: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jsiirola's thought was that in both these disjunctions you are just choosing a value for unitsInPhase and unitsOutOfPhase, so you could just formulate it that way rather than having the disjunctions. But I guess that might make it less useful as a GDP test case?
""" | ||
|
||
alias('core.add_slack_variables', \ | ||
doc="Create a model where we had slack variables to every constraint " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo. "where we add"
pyomo/gdp/disjunct.py
Outdated
elif hasattr(arg, '__getitem__'): | ||
return (_Initializer.dict_like, arg) | ||
else: | ||
# Hopefully this thing is castable to teh type that is desired |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: "the"
pyomo/gdp/disjunct.py
Outdated
|
||
class GDP_Error(Exception): | ||
"""Exception raised while processing GDP Models""" | ||
|
||
|
||
# THe following should eventually be promoted so that all | ||
# IndexedComponents can use it | ||
class _Initializer(object): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some short description?
@@ -1,16 +1,16 @@ | |||
[ 0.00] Setting up Pyomo environment | |||
[ 0.00] Applying Pyomo preprocessing actions | |||
[ 0.00] Creating model | |||
[ 0.01] Creating model |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This broad comment applies to all the book examples. Since changes were made to the book examples, we need to make sure that something is added to the Pyomo website. I recommend we add a link and some description below the Pyomo book on the Documentation page. I will add an issue, and we can discuss it in the pyomo meeting.
@@ -9,15 +9,16 @@ | |||
# ___________________________________________________________________________ | |||
|
|||
import pyomo.core.plugins.transform.relax_integrality | |||
#import pyomo.core.plugins.transform.eliminate_fixed_vars | |||
#import pyomo.core.plugins.transform.standard_form | |||
# import pyomo.core.plugins.transform.eliminate_fixed_vars |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for these commented out imports?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think at some point some of the transformations were deprecated and thus commented out. This PR doesn't change that fact, other than a cosmetic addition of a space after the #
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I see. In my view it looked like they were added and then commented out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that @whart222 "deprecated" them by removing their registration and seeing if anyone complained.
# packages are optional and/or under development. | ||
# | ||
_optional_packages = set([ | ||
'pyomo.contrib.example', | ||
'pyomo.contrib.preprocessing']) | ||
'pyomo.contrib.preprocessing', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these _optional_packages used for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _optional_packages
import the necessary contrib modules, such as the transformation plugins that I have under pyomo.contrib.preprocessing
. The comment lines above describe its functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, the comment is confusing since it says they "need to be loaded", but also says that they "are optional". No biggie. I am fine with it.
@@ -20,7 +20,7 @@ | |||
from pyomo.gdp import * | |||
from pyomo.repn import generate_canonical_repn | |||
|
|||
logger = logging.getLogger('pyomo.core') | |||
logger = logging.getLogger('pyomo.gdp') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great.
@@ -39,7 +39,7 @@ def _apply_solver(self): | |||
xfrm.apply_to(self._instance) | |||
|
|||
xfrm = TransformationFactory('gdp.bigm') | |||
xfrm.apply_to(self._instance, default_bigM=self.options.get('bigM',10**6)) | |||
xfrm.apply_to(self._instance, bigM=self.options.get('bigM',10**6)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally minor, but why 10**6 vs 100000 used in an earlier file as the default? (1) I think it was 1e6 vs 1e5, but I am not sure. Also, what is wrong with 1e5? or 1e6?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that @whart222 wrote this code and that was his style. I agree that 1e6 would be better.
Key : Lower : Body : Upper : Active | ||
None : 1.0 : cc.expr1.indicator_var + cc.expr2.indicator_var + cc.expr3.indicator_var : 1.0 : True | ||
Key : Disjuncts : Active : XOR | ||
None : ['cc.expr1', 'cc.expr2', 'cc.expr3'] : True : True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This move to list from a sum is expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a move from a Constraint-like
to a list, with a flag for whether it's an XOR disjunction vs. an OR disjunction. It introduces support for OR disjunctions, whereas the old approach only supported XOR disjunctions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes: this is expected. The original GDP implementation hijacked standard Constraints to implement the Disjunction (because at the time deriving Components was even more convoluted than it is now). One of the big points of the rewrite was to make GDP use "standard" components. The move to a list of disjuncts instead of the MIP equivalent is more intuitive / informative from the modeling perspective.
@@ -159,7 +159,7 @@ def _warm_start(self, instance): | |||
mst_file.write("<header/>\n") | |||
mst_file.write("<quality/>\n") | |||
mst_file.write("<variables>\n") | |||
for var in instance.component_data_objects(Var, active=True): | |||
for var in instance.component_data_objects(Var): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to loop through variables that are not active?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out that Vars aren't active (they are not ActiveComponents and cannot be deactivated). The active flag only controlled if the generator descended into all blocks or only active blocks. This change allows the generator to descend into all blocks - not just the active ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. (We just had this conversation :)
@@ -179,7 +179,7 @@ def _warm_start(self, instance): | |||
smap = instance.solutions.symbol_map[self._smap_id] | |||
byObject = smap.byObject | |||
with open(self._warm_start_file_name, 'w') as mst_file: | |||
for vdata in instance.component_data_objects(Var, active=True): | |||
for vdata in instance.component_data_objects(Var): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above.
@@ -139,7 +139,7 @@ def _populate_glpk_instance ( self, model ): | |||
if objective.is_minimizing(): sense = GLP_MIN | |||
|
|||
constraint_list = model.component_map(Constraint, active=True) | |||
variable_list = model.component_map(Var, active=True) | |||
variable_list = model.component_map(Var) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
Codecov Report
@@ Coverage Diff @@
## master #354 +/- ##
==========================================
+ Coverage 65.71% 65.72% +<.01%
==========================================
Files 467 474 +7
Lines 60729 62289 +1560
==========================================
+ Hits 39906 40937 +1031
- Misses 20823 21352 +529
Continue to review full report at Codecov.
|
Fixes #318, #241
Summary/Motivation:
This is a rewrite of the
pyomo.gdp
package.Changes proposed in this PR:
Disjunct
andDisjunction
components to bring them in line with current Component conventions.pyomo.contrib
)Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: