Skip to content

Conversation

@emma58
Copy link
Contributor

@emma58 emma58 commented Nov 24, 2021

Fixes #2101, #2171

Summary/Motivation:

GDPopt calls a series of transformations from contrib.preprocessing on the NLP subproblems prior to solving them, and it assumes that if they are found infeasible during preprocessing, those transformations will throw an InfeasibleConstraintException. However, many of them threw a ValueError when they found infeasible constraints. This PR modifies them all to throw an InfeasibleConstraintException. In addition, it fixes a bug in contrib.propagate_fixed_vars where constraints of the form x == y + constant were mistakenly treated as x == y, and could result in fixing variables to infeasible values.

Changes proposed in this PR:

  • Changes ValueErrors in contrib.propagate_fixed_vars, contrib.deactivate_trivial_constraints, and contrib.propagate_eq_var_bounds to be InfeasibleConstraintExceptions
  • Modifies contrib.propagate_fixed_vars to ignore constraints of the form x == y + constant when one of x or y is not already fixed prior to the transformation call. This is consistent with what the docstring promises and fixes the silent failure from Propagate_fixed_vars transformation does not handle constants in standard repn #2101, so it seems worth doing for now. However, I think that we should have a discussion about what behavior we actually want: maybe we should handle these types of constraints.

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:

  1. I agree my contributions are submitted under the BSD license.
  2. 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.

…traints in contrib.deactivate_trivial_constraints, which fixes Pyomo#2171
…l_constraints should actually raise an InfeasibleConstraintException, rather than a ValueError...
…h the gdpopt set covering initialization, where equality constraints are infeasible. I think I'm barking up the wrong tree.
…r conflicting fixed values in propogate_fixed_vars
…straintErrors rather than ValueErrors when they encounter infeasible constraints. This is consistent with what GDPopt expects when it encounters infeasible subproblems.
…ts. Not in a clever way, as it just ignores equality constraints with constants for the moment, which is actually what the docstring promises. Also fixes tests that were expecting ValueErrors where now there are InfeasibleConstraintExceptions
…raintExceptions, logging the exceptions because I think that might be helpful
@codecov
Copy link

codecov bot commented Nov 24, 2021

Codecov Report

Merging #2211 (0178ffb) into main (f8c4b04) will increase coverage by 0.15%.
The diff coverage is 84.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2211      +/-   ##
==========================================
+ Coverage   82.99%   83.14%   +0.15%     
==========================================
  Files         607      607              
  Lines       76774    77578     +804     
==========================================
+ Hits        63718    64503     +785     
- Misses      13056    13075      +19     
Impacted Files Coverage Δ
pyomo/contrib/gdpopt/master_initialize.py 94.49% <ø> (ø)
pyomo/contrib/gdpopt/nlp_solve.py 72.34% <75.00%> (+3.41%) ⬆️
...ontrib/preprocessing/plugins/equality_propagate.py 98.13% <75.00%> (+0.01%) ⬆️
pyomo/contrib/mindtpy/nlp_solve.py 86.59% <100.00%> (+0.07%) ⬆️
...ocessing/plugins/deactivate_trivial_constraints.py 93.33% <100.00%> (+0.15%) ⬆️
pyomo/contrib/mindtpy/single_tree.py 51.16% <0.00%> (-2.64%) ⬇️
pyomo/common/unittest.py 92.19% <0.00%> (-0.41%) ⬇️
pyomo/core/expr/compare.py 100.00% <0.00%> (ø)
pyomo/util/calc_var_value.py 100.00% <0.00%> (ø)
pyomo/core/expr/numeric_expr.py 98.73% <0.00%> (+0.24%) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8c4b04...0178ffb. Read the comment docs.

Copy link
Member

@jsiirola jsiirola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine, but we should re-implement the transformation to handle non-zero constants in the trivial linear equality expressions.

If we merge this without fixing the transformation, we should at least create an issue to revisit the problem. I think fixing the transformation should only take ~15 minutes, though.

@emma58
Copy link
Contributor Author

emma58 commented Dec 2, 2021

@jsiirola I'm going to merge this and open an issue about the transformation. Because what to do about constants depends on how we resolve #2204, I think. Because currently a constraint like x == y + constant will get handled as desired if y is fixed because it looks like x == another_constant after generate_standard_repn with compute_values=True.

@emma58 emma58 merged commit f914866 into Pyomo:main Dec 2, 2021
@emma58 emma58 deleted the set-covering-init-bug branch December 2, 2021 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Propagate_fixed_vars transformation does not handle constants in standard repn

3 participants