Skip to content

Fix modprodnet distributed pair activation - #141

Open
bernalde wants to merge 2 commits into
mainfrom
fix/issue-72-modprodnet-gloa
Open

bernalde wants to merge 2 commits into
mainfrom
fix/issue-72-modprodnet-gloa

Conversation

@bernalde

Copy link
Copy Markdown
Member

Summary

  • Fixes the distributed modprodnet site-pair inactive disjunct so it represents "not both sites active" instead of forcing both sites inactive.
  • Applies the same correction to the quarterly distributed formulation.
  • Adds focused regression coverage for all public modprodnet cases, 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=short
    • Result: 24 passed in 4.74s
  • /home/bernalde/.pixi/bin/pixi run pytest tests/test_module_imports.py -v --tb=short
    • Result: 68 passed in 6.43s
  • /home/bernalde/.pixi/bin/pixi run python - <<'PY' ... PY
    • Direct GDPopt GLOA check for default modprodnet using GAMS-local role solvers (nlp=ipopth, mip=gurobi, minlp=dicopt, local_minlp=dicopt), 60s time limit.
    • Result: status ok, termination optimal, objective/lower/upper bound 3592.9243356537468.
  • /home/bernalde/.pixi/bin/pixi run test
    • Result: 308 passed, 1 skipped in 26.44s
  • /home/bernalde/.pixi/bin/pixi run lint
    • Result: exit 0. Black, critical flake8, and typos passed; the repository's existing non-fatal flake8 --exit-zero style inventory was printed.
  • git diff --check
    • Result: no whitespace errors.

Notes

  • I did not run the full solver-backed benchmark campaign. A narrow gdplib-benchmark run --instances modprodnet --strategies gdpopt.gloa --timelimit 60 --run-id issue72_gloa_probe --solver-profile gams-local probe produced an optimal per-case result, but the benchmark CLI exited during summary generation because generate_benchmark_summary_all is not available in this checkout. The direct GDPopt GLOA solve above was used as the clean solver-backed verification.
  • Issue Plan PyPI release workflow and Pixi platform support #104 is closed release/Pixi follow-up work; this PR intentionally does not change Pixi platform or release metadata.

Closes #72

@bernalde
bernalde force-pushed the fix/issue-72-modprodnet-gloa branch from a777b7a to c3b59d4 Compare May 14, 2026 16:19
@bernalde

Copy link
Copy Markdown
Member Author

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 — modprodnet has a native maximization objective, but this PR's verification used GDPopt GLOA, which was never affected (the hardcoded minimize sense existed only in loa.py). The evidence in this PR stands. The LOA row for modprodnet does need re-validation against a Pyomo build containing the fix, but that is tracked in #72 and independent of this PR.

@bernalde bernalde left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

@bernalde

Copy link
Copy Markdown
Member Author

Addressed the review of head c3b59d4 in commit 88de134:

  • parametrize-pairs: test_pair_inactive_represents_not_both_sites_active now runs over all three unique site pairs (1,2), (1,3), (2,3) for both distributed cases — 40 tests total, up from 24.
  • constraints-satisfied-scope: _constraints_satisfied now carries the scope comment about walking every active block constraint.
  • redundant-no-transfer (question): keeping the pair-level no_module_transfer is deliberate — it is logically redundant with the site-level constraint but tightens the continuous relaxation; both distributed.py and quarter_distributed.py now record that intent in a comment.

On the evidence note from the review: a Distributed-case GLOA probe remains the open gap; issue #72 should stay open (Refs #72 reading) until a solver run on a changed variant lands, and previously recorded Distributed/QuarterDistributed baselines need re-baselining against the enlarged feasible region.

Tests: pixi run pytest tests/test_modprodnet.py — 40 passed; black --check clean on the three changed files. CI for the new head was starting when this was posted; the previous head was green (6/6).

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.

Refactor modprodnet to fix benchmark issues

1 participant