Bugfix: lp_writer SOS constraints and row_order#3678
Conversation
| model = pyo.ConcreteModel() | ||
|
|
||
| model.index_set = pyo.Set(initialize=[1, 2]) | ||
| model.idx_set = pyo.Set(initialize=[1, 2]) |
There was a problem hiding this comment.
This example clearly was not being tested. Is it now?
There was a problem hiding this comment.
I suspect a large chunk of our examples are not being tested. That's a bigger effort than this PR. If you want me to cobble something together for this exact example, I can, but I'd recommend that we turn it into a targeted PEP instead.
There was a problem hiding this comment.
'Eyyyy, this is, in fact, already a PEP: #2962
| def ordered_active_constraints(model, config): | ||
| sorter = FileDeterminism_to_SortComponents(config.file_determinism) | ||
| constraints = model.component_data_objects(Constraint, active=True, sort=sorter) | ||
|
|
||
| row_map = row_order2row_map(config) |
There was a problem hiding this comment.
While it is good to avoid the duplicate code, from an efficiency / consistency standpoint, this is less than ideal (row_order2row_map is being called twice. I think I would add row_map as an argument of this function.
There was a problem hiding this comment.
I would like to go through the standard form, LP, and NL writers and refactor this logic to remove redundancy & enforce uniformity (e.g., I am pretty sure that the NLv2 is ignoring row_order!). But, I am comfortable merging this PR first. The redundant call to row_order2row_map only hits users with both SOSConstraint components and who are specifying a row_order (and realistically, it's not that big of a hit).
github-project-automation
bot
moved this from Review In Progress
to Reviewer Approved
in August 2025 Release
github-project-automation
bot
moved this from Reviewer Approved
to Done
in August 2025 Release
3 participants
Fixes NA
Summary/Motivation:
In starting work on the new
bar_writer, I stumbled across a few bugs inlp_v2that no one has hit yet but definitely don't work. Primarily, the bugs were:row_orderin the writer config.Changes proposed in this PR:
ordered_active_constraintsto pull outrow_maplogic (for reusability)row_order2row_mapfunctionindex_setis now a reserved name, so SOS example fixed so it'll actually runLegal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: