Comments you submit will be routed for moderation. If you have an account, please log in first.
Modify

Ticket #4214 (closed major enhancement: fixed)

Opened 3 years ago

Last modified 3 years ago

Change variable ordering in rules

Reported by: jdsiiro Owned by: wehart
Priority: major Milestone: Coopr 3.0
Component: coopr.pyomo Version:
Keywords: Cc: jwatson

Description

OK, so I realize that this appears to be a very disruptive change... but it has been bothering me for a while now, and I now have hard data as to why this is really not a good practice -- plus a solution that maintains full backwards compatibility.

The problem: Why are all rules defined as indices first, then the model?

  1. This is counter-intuitive from a programming standpoint: in all languages (including Python) you always put the variable-length arguments at the end of the argument list. That is, we should do "model, I, J, ...".
  1. This is SLOWER. We spend a measurable amount of time (one-third of constraint.construct()'s direct time) taking the indices, converting them to a list, adding the model to the end, and converting back to a tuple to pass to the rule.
  1. This places a restriction on rules: either all invocations of a rule must use the same number of indices, or the rule must explicitly take *args and remove model from the end itself. If the model came first, they could define the rule as: "def _rule(model, *index):"

Here's the best part: we can change the semantics and still maintain backwards compatibility using a try-catch block (since transposition of model and index is virtually guaranteed to raise an exception). Old-style models would still run (albeit, slightly slower), but new-style models would be both cleaner and faster.

Timings (constraint.construct() for pmedian.test7.dat):

original: 0.370 sec
new with old-style rule: 0.566 sec
new with new-style rule: 0.246 sec

Proposed change (implemented within constraint.construct):

if index.__class__ is tuple:
    try:
        expr = _self_rule(self.model, *index)
    except:
        tmp = list(index)
        tmp.append(self.model)
        try:
             expr = _self_rule(*tmp)
        except:
             # Re-run new style and allow exception to propagate
             expr = _self_rule(self.model, *index)
else:
    try:
        expr = _self_rule(self.model, index)
    except:
        try:
             expr = _self_rule(index, self.model)
        except:
             # Re-run new style and allow exception to propagate
             expr = _self_rule(self.model, index)
if expr is not None:
    self.add(index, expr)

Attachments

Change History

comment:1 Changed 3 years ago by wehart

  • Milestone changed from Coopr 2.5 to Coopr 2.5.1

John:

Given our recent discussion, I'd rather leaves this change for the 2.5.1 patch release. I'm updating this ticket accordingly.

comment:2 Changed 3 years ago by wehart

  • Milestone changed from Coopr 2.5.1 to Coopr 3.0

comment:3 Changed 3 years ago by wehart

  • Owner changed from wehart to jdsiiro
  • Status changed from new to assigned

I'm pretty sure that this change hasn't happened. I'd like to see this happen sooner rather than later so we can have the Coopr book document this effect. John?

comment:4 Changed 3 years ago by jdsiiro

These changes have not been made yet. They are relatively simple (90% is already in the ticket). The time consuming part is adding the necessary tests and updating all our models / examples to show the new format.

Unfortunately, I am in crisis mode for other projects, and will not have a chance to get to this until June 1.

comment:5 Changed 3 years ago by wehart

  • Owner changed from jdsiiro to wehart

There are actually many more changes to make than in the constraint.py file. I am assigning this ticket to myself to ensure that all of the appropriate changes are made.

comment:6 Changed 3 years ago by wehart

Referenced in changeset [4039]:

This commit is a significant change to the API of coopr.pyomo classess. The default API for construction rules, validation functions, filter functions, etc has been changed to put the model arguement first.

The previous API placed indices first and then the model. This has several problems:

. This is counter-intuitive from a programming standpoint: in all languages (including Python) you always put the variable-length arguments at the end of the argument list. That is, we should do "model, I, J, …".

. This is SLOWER. We spend a measurable amount of time setting up the arguments for rules that place the model last. The indices are converted to a list, the model is added to the end, and this list is converted back to a tuple to pass to the rule.

. This places a restriction on rules: either all invocations of a rule must use the same number of indices, or the rule must explicitly take *args and remove model from the end itself. If the model came first, users could define the rule as: "def _rule(model, *index):"

This commit changes the semantics while still maintaining a degree of backwards compatibility. We use try-except blocks to detect when a rule fails due to an incorrect argument sequence. In this manner, Old-style models will often still run (albeit, slightly slower), but new-style models would be both cleaner and faster.

Currently, a warning log message is generated when an old-style rule is used. Later, we can make this an error log (in Coopr 3.1), before removing the try-except block altogether in future Pyomo releases (in Coopr 4.0).

This commit resolves #4214 and #4163

comment:7 Changed 3 years ago by wehart

  • Status changed from assigned to closed
  • Resolution set to fixed

comment:8 Changed 3 years ago by jdsiiro

Referenced in changeset [4194]:

Switching all places where we call rules to use a common helper function so that we are consistent in how we manage index tuples and the fallback to the pre-3.0-style rule format (model last instead of model first).

This fixes a common complaint due to the partial implementation of #4214 where errors in rules were reported in the old-style context and not the new-style context.

View

Add a comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
The resolution will be deleted. Next status will be 'reopened'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.