Separate Linear Terms in Nonlinear to Piecewise Linear Transformation#3814
Separate Linear Terms in Nonlinear to Piecewise Linear Transformation#3814michaelbynum merged 16 commits intoPyomo:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3814 +/- ##
==========================================
- Coverage 89.94% 89.93% -0.01%
==========================================
Files 902 902
Lines 106354 106393 +39
==========================================
+ Hits 95655 95680 +25
- Misses 10699 10713 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
emma58
left a comment
There was a problem hiding this comment.
I think this makes sense. If you could put in a comment with the example you put in the PR description for why _separate_linear_parts is necessary, I think that would be helpful for future us. My guess is this is not currently the fastest thing we could possibly do, but for now, I'm not so concerned with that. @jsiirola, @sadavis1, @bammari, this might be worth glancing at if you have a minute.
| if x1 == x2: | ||
| nonlinear += coef * var_map[x1] ** 2 | ||
| else: | ||
| nonlinear += coef * (var_map[x1] * var_map[x2]) |
There was a problem hiding this comment.
Does separating these two cases matter? I mean, obviously you make a different expression tree, but do we need it?
There was a problem hiding this comment.
I don't think it matters to me.
There was a problem hiding this comment.
I'm going to leave this just for consistency with QuadraticRepn.
| if repn.multiplier_flag(repn.multiplier) != 1: | ||
| linear *= repn.multiplier | ||
| nonlinear *= repn.multiplier |
There was a problem hiding this comment.
@jsiirola, can this happen? I have something in the back of my mind telling me multiplier is sure to be 1 at this point?
There was a problem hiding this comment.
I just copied the to_expression code from the QuadraticRepn class and modified it slightly...
There was a problem hiding this comment.
which is to say that I have no idea. I'll take a look, though.
There was a problem hiding this comment.
Oh, I can't read. This question was for @jsiirola. My bad.
|
Okay, if this does make some sense, I'll clean it up a bit. |
|
Okay, assuming tests pass, this should be good to go. |
| linear *= repn.multiplier | ||
| nonlinear *= repn.multiplier | ||
|
|
||
| return linear, nonlinear |
There was a problem hiding this comment.
let's not lump the quadratic part back into the nonlinear part. additively decompose will separate it again.
There was a problem hiding this comment.
I started to do this, but the min_dimension_to_additively_decompose option made it more complex than I thought was worthwhile.
There was a problem hiding this comment.
I'm taking back what I said. I'm going to make this change. I was confused.
There was a problem hiding this comment.
Okay, this is done, now.
|
Assuming tests pass, this should be ready for review. |
|
So this is not ready for review yet. |
|
Assuming tests pass, this should be ready for review. |
jsiirola
left a comment
There was a problem hiding this comment.
A couple minor questions / suggestions....
| 0 >= PWL(exp(x) + x**3 - y) | ||
| """ | ||
| assert repn.multiplier == 1 | ||
| linear_repn = QuadraticRepn() |
There was a problem hiding this comment.
If this will always just have constant + linear, why not use LinearRepn?
There was a problem hiding this comment.
It feels weird to use a LinearRepn with a QuadraticRepnVisitor?
There was a problem hiding this comment.
Maybe. But I suppose no weirder than
linear_repn = QuadraticRepn()There was a problem hiding this comment.
So true... I should not be allowed to name things.
Summary/Motivation:
This PR modifies the
contrib.piecewise.nonlinear_to_pwltransformation so that linear parts of a constraint always get separated from the nonlinear parts, even ifadditively_decomposeisFalse. The idea is thatshould become
and not
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: