Transformation of explicit constraints to variable bounds #190
Conversation
Codecov Report
@@ Coverage Diff @@
## master #190 +/- ##
=========================================
+ Coverage 66.17% 66.2% +0.02%
=========================================
Files 442 443 +1
Lines 56759 56803 +44
=========================================
+ Hits 37561 37605 +44
Misses 19198 19198
Continue to review full report at Codecov.
|
| # Check if the constraint is k * x + c1 <= c2 or c2 <= k * x + c1 | ||
| if constr.body.polynomial_degree() == 1: | ||
| repn = generate_canonical_repn(constr.body) | ||
| if repn.variables is not None and len(repn.variables) == 1: |
There was a problem hiding this comment.
Again, you should filter out the variables with a 0 coefficient first before you decide if you can update the bounds.
There was a problem hiding this comment.
I assume that using the > and < operators implicitly do this filtering. Is that not sufficient?
There was a problem hiding this comment.
No: edge cases like in your other transformation (e.g., m.x + m.y - m.x <= 0 will end up with a zero coefficient in the canonical repn - and two variables in the repn.variables. That said, the correct answer may be to have generate_canonical_repn filter those out as part of it's final postprocessing step...
There was a problem hiding this comment.
I think it might be compelling to do that post-processing, since I tend to assume at least for linear systems that the variable terms are aggregated. I don't think that the edge case is relevant to this transformation, though, because of the len(repn.variables) == 1 requirement. It will just fail to transform that constraint.
There was a problem hiding this comment.
Actually, it appears that for your simple example, this kind of processing already takes place, both for coopr3 and pyomo4 expression systems.
| if repn.variables is not None and len(repn.variables) == 1: | ||
| var = repn.variables[0] | ||
| const = repn.constant if repn.constant is not None else 0 | ||
| coef = repn.linear[0] |
There was a problem hiding this comment.
For safety (and Python2 compatibility), you should cast the coef to a float to avoid integer division: coef = float(repn.linear[0])
There was a problem hiding this comment.
Even with the from __future__ import division up top?
There was a problem hiding this comment.
DOH. I missed that. Yes - the __future__ will cover it. [So you are seeing a style difference ... instead of remembering to start every script with the division import, I just trained myself to explicitly convert something to float]
|
Close and reopen to trigger rebuild due to a network-connectivity-related issue. |
|
Close and reopen to work around |
| # variable from all active constraints, so that it won't be | ||
| # updated during the optimization. Therefore, we need to | ||
| # shift the value of var as necessary in order to keep it | ||
| # within the constraints. |
There was a problem hiding this comment.
Several comments:
-
Unclear exactly what this comment is saying. Are you indicating that it is possible that the variable is only in constr, therefore, when we deactivate it, the variable does not appear in any constraints at all?
-
Also, why is it necessary to shift the value? It is possible to have variable values that are outside the bounds. Is this just "cleanup"?
-
If I understand correctly, then "keep it within the constraints" should read "keep it within the bounds".
There was a problem hiding this comment.
- Yep.
- Yes, this is a form of cleanup.
- Yeah, I can change that.
There was a problem hiding this comment.
On further thought, with 2: It makes sense to shift the value because we are removing a constraint from the system and shifting its "effect" directly onto the variable. In order for post-solve variable values to remain as they would be had we not performed the transformation, we need to move the variable value if it falls outside of the new bound.
Adding a new transformation.