Constraint: only store the original expression (not lower/body/upper) #3293

jsiirola · 2024-06-17T18:18:30Z

Fixes # .

Summary/Motivation:

This PR changes the internal data store for Constraint to only store the original (relational) expression and not break it apart into (lower, body, and upper). Those attributes are preserved as properties (and the expression rearrangement happens dynamically when they are called). Most clients who need the expression "standardized" should use the new normalize_constraint() method (which is more efficient, as it avoids duplicate work and unnecessary calls to as_numeric()).

While this would appear to be strictly cosmetic, this change is the first part of the templatized LP writer.

Changes proposed in this PR:

remove _lower, _body, and _upper ConstraintData attributes
enforce the use of .args as the only public API for getting at an expression's arguments
update persistent interfaces to look at the expr (and not body ) to detect changed expressions
update FBBT to use the same relational expression handling logic as compute_bounds_on_expression

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:

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.

…dy, upper)

(something caused gdpopt to set variables to int instead of floats)

pyomo/core/base/constraint.py

codecov · 2024-07-10T02:09:02Z

Codecov Report

Attention: Patch coverage is 90.84967% with 14 lines in your changes missing coverage. Please review.

Project coverage is 88.32%. Comparing base (92a9d3b) to head (2529557).
Report is 4 commits behind head on main.

Files	Patch %	Lines
pyomo/gdp/plugins/bilinear.py	0.00%	4 Missing ⚠️
pyomo/core/base/constraint.py	94.73%	3 Missing ⚠️
pyomo/contrib/fbbt/fbbt.py	96.29%	2 Missing ⚠️
pyomo/gdp/plugins/cuttingplane.py	50.00%	2 Missing ⚠️
pyomo/contrib/appsi/base.py	75.00%	1 Missing ⚠️
pyomo/contrib/solver/persistent.py	80.00%	1 Missing ⚠️
pyomo/core/kernel/constraint.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3293      +/-   ##
==========================================
- Coverage   88.49%   88.32%   -0.18%     
==========================================
  Files         868      868              
  Lines       98436    98419      -17     
==========================================
- Hits        87115    86928     -187     
- Misses      11321    11491     +170

Flag	Coverage Δ
linux	`85.76% <90.84%> (-0.57%)`	⬇️
osx	`75.29% <89.54%> (-0.35%)`	⬇️
other	`86.31% <90.84%> (-0.21%)`	⬇️
win	`83.84% <90.84%> (+0.01%)`	⬆️

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

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

emma58

This mostly looks good (nice sneaky FBBT additions!), but I have quibbles with the naming of normalize_constraint.

emma58 · 2024-07-22T20:45:13Z

pyomo/core/base/constraint.py

+            body = value(self.body, exception=exception)
+        return body
+
+    def normalize_constraint(self):


I'm not sure I like the name of this method... 'Normalize' makes me think of means of 1 and stuff... Could it be tupelize_constraint or something?

I'm not too keen on tuplize_constraint (it doesn't feel very descriptive); but I agree that normalize_constraint is probably worse. What about standardize_constraint or to_range_constraint (the latter is a bit misleading as it does not return a RangedExpression)?

Hmmm. I actually kind of like to_range_constraint... Maybe to_tuple (also not super descriptive, but since we accept Constraint expressions in that form, it kinda makes sense)? I don't love standardize_constraint because of potential standard form connotations, but I do like it a lot better than normalize_constraint!

to_bounded_expression()?

Oooo, I think I like that one!

Implemented!

emma58 · 2024-07-22T20:45:22Z

pyomo/core/base/constraint.py

-        #
-        # Normalize the incoming expressions, if we can
-        #


This is a very beautiful block of deleted code! :D

pyomo/core/kernel/constraint.py

emma58 · 2024-07-22T20:45:32Z

pyomo/core/tests/unit/test_con.py

-        self.assertEqual(model.c.lower, None)
+        self.assertEqual(model.c.lower, float('-inf'))


Is this change backwards compatible? Or were we testing for the existence of a bug?

We were testing for the existence of a bug: By (current) convention, lower returns the expression for the lower bound and lb returns the value (or None if not specified or -inf). The problem was that in the old world order, some bounds processing was done "early" in set_value -- including some of the conversion from +/-inf to None. We now defer that processing until the user asks for the value. So, in this test, since the user provided the "expression" float('-inf') for the lower bound, the "correct" thing to do is return that expression from lower.

pyomo/contrib/fbbt/tests/test_fbbt.py

mrmundt

I had one minor nitpick about a missed close-parenthesis but otherwise, I'm happy with it!

blnicho

I suspect that this section in pyomo.contrib.sensitivity_toolbox can also be simplified with this change:

pyomo/pyomo/contrib/sensitivity_toolbox/sens.py

Line 693 in 1ada528

# Constraint must be a ranged inequality, break into

blnicho · 2024-07-26T23:15:02Z

pyomo/core/tests/unit/test_con.py

-
-        def rule(model):
-            return (float('inf'), model.x)
+        model = self.create_model(abstract=True).create_instance()


Why not change this to just create a ConcreteModel? Are there behaviors we expect with AbstractModels that are no longer being tested because of this change?

I think we could just switch this over to a ConcreteModel. I left it as it was mostly to minimize changes in old code / tests. The biggest reason for the change here was to remove the test for an exception on model creation, as this PR defers the bounds sanity checking until later (model compilation step and not model construction step).

blnicho · 2024-07-26T23:15:50Z

pyomo/core/tests/unit/test_con.py

-
-        model.c = Constraint(rule=rule)
-        self.assertRaises(ValueError, model.create_instance)
+        model = self.create_model(abstract=True).create_instance()


Same question about just using ConcreteModel.

Same as above: we certainly could change these tests, but I didn't to minimize (unnecessary) changes to old code.

pyomo/core/base/constraint.py

Co-authored-by: Bethany Nicholson <blnicho@users.noreply.github.com>

jsiirola · 2024-07-29T14:13:37Z

@blnicho: I went ahead and updated the logic in sens.py, but did not rework the use of abstract models in the old Constraint tests

blnicho · 2024-07-29T15:01:54Z

@jsiirola I don't see the changes to sens.py. Did you forget to push?

…nly' into constraint-store-expr-only

jsiirola · 2024-07-29T19:14:45Z

@jsiirola I don't see the changes to sens.py. Did you forget to push?

Yup. Well - no. I ran git push but it was rejected because I had clicked the "update branch" button and I didn't notice. .

… from Pyomo#3293

jsiirola and others added 12 commits June 8, 2024 17:11

Update ConstraintData to only store the expression (and not lower, bo…

f4a17e1

…dy, upper)

Update FBBT to work with raw relational expressions

25027f9

Update transformations to not rely on Constraint lower/body/upper

1b20539

Update solver interfaces to not rely on Constraint lower/body/upper

8714e4c

NFC: apply black

7aee9e0

Switch from caching .body to caching .expr

3d77bc9

Update APPSI cmodel to call normalize_constraint()

d8de810

Remove repeated function

496a663

Switch logic for mapping lower/body/upper to match previous behavior

0b0833f

Update baseline due to changes in Constraint

cdfb2b1

(something caused gdpopt to set variables to int instead of floats)

NFC: improve variable naming

64e01bb

Merge branch 'main' into constraint-store-expr-only

f96d325

blnicho requested a review from emma58 June 25, 2024 19:00

blnicho self-requested a review July 2, 2024 19:08

Merge branch 'main' into constraint-store-expr-only

8f30c3d

mrmundt reviewed Jul 9, 2024

View reviewed changes

pyomo/core/base/constraint.py Outdated Show resolved Hide resolved

mrmundt reviewed Jul 9, 2024

View reviewed changes

pyomo/core/base/constraint.py Show resolved Hide resolved

NFC: update spaces in message

f658981

jsiirola added 3 commits July 10, 2024 10:59

Fix (and test) FBBT error with RangedExpressions

533f616

Only test variable RangedExpressions in Python FBBT

399c731

Move to using expr (not body) when gathering variabels from constraints

7f84d3e

emma58 reviewed Jul 22, 2024

View reviewed changes

Rename normalize_constraint -> to_bounded_expression

08ccf40

jsiirola requested review from emma58 and mrmundt July 24, 2024 16:58

Merge branch 'main' into constraint-store-expr-only

f5ba8a2

mrmundt reviewed Jul 24, 2024

View reviewed changes

pyomo/contrib/fbbt/tests/test_fbbt.py Outdated Show resolved Hide resolved

mrmundt approved these changes Jul 24, 2024

View reviewed changes

NFC: fix comment typo

2529557

emma58 approved these changes Jul 24, 2024

View reviewed changes

Merge branch 'main' into constraint-store-expr-only

2d326ea

blnicho reviewed Jul 26, 2024

View reviewed changes

jsiirola and others added 4 commits July 29, 2024 07:46

NFC: fix doc typo

49a53d8

Co-authored-by: Bethany Nicholson <blnicho@users.noreply.github.com>

NFC: fix doc typo

2150295

Co-authored-by: Bethany Nicholson <blnicho@users.noreply.github.com>

Remove redundant header, simplify imports

d8900b2

Update constraint processing to leverage new Constraint internal storage

bb17e9c

Merge remote-tracking branch 'refs/remotes/me/constraint-store-expr-o…

f5ff0a1

…nly' into constraint-store-expr-only

blnicho approved these changes Jul 29, 2024

View reviewed changes

blnicho merged commit f46da7f into Pyomo:main Jul 30, 2024
32 checks passed

jsiirola deleted the constraint-store-expr-only branch August 2, 2024 01:12

jsiirola added a commit to jsiirola/pyomo that referenced this pull request Aug 13, 2024

Update writers to use to_bounded_expression; recover performance loss…

d29d3db

… from Pyomo#3293

jsiirola mentioned this pull request Aug 13, 2024

Resolve writer performance degradation #3343

Merged

bknueven pushed a commit to bknueven/pyomo that referenced this pull request Aug 15, 2024

Update writers to use to_bounded_expression; recover performance loss…

c07be8c

… from Pyomo#3293

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Constraint: only store the original expression (not lower/body/upper) #3293

Constraint: only store the original expression (not lower/body/upper) #3293

jsiirola commented Jun 17, 2024

codecov bot commented Jul 10, 2024 •

edited

Loading

emma58 left a comment

emma58 Jul 22, 2024

jsiirola Jul 22, 2024

emma58 Jul 23, 2024

jsiirola Jul 23, 2024

emma58 Jul 23, 2024

jsiirola Jul 24, 2024

emma58 Jul 22, 2024

emma58 Jul 22, 2024

jsiirola Jul 24, 2024

mrmundt left a comment

blnicho left a comment

blnicho Jul 26, 2024

jsiirola Jul 29, 2024

blnicho Jul 26, 2024

jsiirola Jul 29, 2024

jsiirola commented Jul 29, 2024

blnicho commented Jul 29, 2024

jsiirola commented Jul 29, 2024

		self.assertEqual(model.c.lower, None)
		self.assertEqual(model.c.lower, float('-inf'))

Constraint: only store the original expression (not lower/body/upper) #3293

Constraint: only store the original expression (not lower/body/upper) #3293

Conversation

jsiirola commented Jun 17, 2024

Fixes # .

Summary/Motivation:

Changes proposed in this PR:

Legal Acknowledgement

codecov bot commented Jul 10, 2024 • edited Loading

Codecov Report

emma58 left a 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

mrmundt left a comment

Choose a reason for hiding this comment

blnicho left a 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

jsiirola commented Jul 29, 2024

blnicho commented Jul 29, 2024

jsiirola commented Jul 29, 2024

codecov bot commented Jul 10, 2024 •

edited

Loading