Conversation
a777b7a to
c3b59d4
Compare
|
Objective-sense note (context: Pyomo/pyomo#3986, merged 2026-08-13, fixed the GDPopt LOA objective-sense bug tracked in #114): No changes needed here — |
bernalde
left a comment
There was a problem hiding this comment.
Maintainer review of head c3b59d4.
The fix is correct and minimal. At base, the XOR pair disjunction admitted only both-active or both-inactive per site pair, so across the three unique pairs only "no sites" or "all three sites" active survived — partial activation was infeasible. The head's pair_inactive (y1 + y2 <= 1 plus zero transfers) is exactly the logical complement of pair_active, and the change strictly enlarges the feasible region, so distributed-case optima can only weakly improve. No objective leakage: dist_to_site is unconstrained under pair_inactive but multiplies modules_transferred = 0. The regression test genuinely fails on base for the (1,0)/(0,1) rows.
Findings:
Nonblocking: single-pair coverage — test_pair_inactive_represents_not_both_sites_active exercises only pair (1, 2). Parametrize over the three unique site pairs so the whole disjunct family is covered at negligible cost.
Nonblocking: helper scope comment — _constraints_satisfied walks all active constraints on the block; fine today, but add a comment that it assumes every block constraint is evaluable at the set values, so future block additions do not silently change the test's scope.
Question: redundant constraint kept — no_module_transfer inside pair_inactive is logically redundant (any inactive site's own site_inactive[site].no_module_transfer already zeroes those transfers) but tightens relaxations. Deliberate? No change required either way; a one-line comment would record the intent.
Evidence note: the body's "Direct GDPopt GLOA check for default modprodnet" solved the Growth case — a model this PR does not touch — so the claimed solver-backed verification does not exercise the changed Distributed/QuarterDistributed variants. Also, the generate_benchmark_summary_all import failure mentioned in the body is not reproducible at head (the CLI imports generate_benchmark_summary from gdplib.benchmark_summary, which exists); it was almost certainly a stale installed console script, not a repo defect.
Issue linkage: issue #72's GLOA row is unchecked and no solver evidence covers the changed variants; recorded Distributed/QuarterDistributed baselines predate the enlarged feasible region and need re-baselining. Treat "Closes #72" as Refs #72 until a GLOA run on a distributed case lands.
Summary — Blocking: 0. Nonblocking: 2. Questions: 1. Tests: static verification of the disjunct algebra and red-on-base reasoning; "24 passed" matches the parametrization count; CI is green at this head (6/6 checks). Merge-ready; a distributed-case solver probe would close the evidence gap. Posting as COMMENT (author account); the main ruleset requires one approving review from another maintainer.
|
Addressed the review of head c3b59d4 in commit 88de134:
On the evidence note from the review: a Distributed-case GLOA probe remains the open gap; issue #72 should stay open ( Tests: |
Summary
modprodnetsite-pair inactive disjunct so it represents "not both sites active" instead of forcing both sites inactive.modprodnetcases, Big-M/Hull reformulation smoke tests, invalid case handling, and the corrected pair-inactive semantics.Tests run
/home/bernalde/.pixi/bin/pixi run pytest tests/test_modprodnet.py -v --tb=short24 passed in 4.74s/home/bernalde/.pixi/bin/pixi run pytest tests/test_module_imports.py -v --tb=short68 passed in 6.43s/home/bernalde/.pixi/bin/pixi run python - <<'PY' ... PYmodprodnetusing GAMS-local role solvers (nlp=ipopth,mip=gurobi,minlp=dicopt,local_minlp=dicopt), 60s time limit.ok, terminationoptimal, objective/lower/upper bound3592.9243356537468./home/bernalde/.pixi/bin/pixi run test308 passed, 1 skipped in 26.44s/home/bernalde/.pixi/bin/pixi run lintflake8 --exit-zerostyle inventory was printed.git diff --checkNotes
gdplib-benchmark run --instances modprodnet --strategies gdpopt.gloa --timelimit 60 --run-id issue72_gloa_probe --solver-profile gams-localprobe produced an optimal per-case result, but the benchmark CLI exited during summary generation becausegenerate_benchmark_summary_allis not available in this checkout. The direct GDPopt GLOA solve above was used as the clean solver-backed verification.Closes #72