Fix biofuel site module constraints

[bernalde]

Summary

• Fix biofuel conventional and inactive site disjuncts so their no_modules constraints only reference module variables for the disjunct's own site.
• Add focused regression tests that verify those constraints are site-local and that biofuel still reformulates with gdp.bigm and gdp.hull.

Tests run

• pixi run pytest tests/test_biofuel.py -v --tb=short - passed, 4 passed.
• pixi run pytest 'tests/test_module_imports.py::TestModelConstruction::test_model_construction[biofuel]' -v --tb=short - passed, 1 passed.
• pixi run test - passed, 288 passed and 1 skipped.
• pixi run lint - exited 0 after Black, critical flake8, non-blocking full flake8 report, and typos.
• git diff --check - passed.

Notes

• No solver-backed benchmark campaign was run locally; the default test coverage remains solver-free and this change is limited to model algebra and GDP transformation smoke tests.
• Considered Plan PyPI release workflow and Pixi platform support #104 while keeping release and Pixi platform policy out of scope for this model fix.

Closes #62

[bernalde]

Blocking issues: None.

Nonblocking issues: None.

Questions: None.

Tests run and outcomes:

• git diff --check origin/main...HEAD passed.
• /home/bernalde/.pixi/bin/pixi run pytest tests/test_biofuel.py tests/test_module_imports.py -v --tb=short passed, 72 passed.
• /home/bernalde/.pixi/bin/pixi run test passed, 288 passed and 1 skipped.
• /home/bernalde/.pixi/bin/pixi run lint passed. The configured non-blocking full flake8 report still printed existing style findings under --exit-zero.
• gh pr checks 133 shows all CI checks passing.

This PR should be merged as-is based on the reviewed diff and checks. I am authenticated as bernalde, which is also the PR author, so I am submitting this as a COMMENT review rather than APPROVE; an eligible reviewer still needs to approve it.

[bernalde]

Benchmark evidence from a Gurobi-only biofuel run on this PR branch (2026-05-12).

I skipped gdpopt.enumerate because the model has 252 disjunctions and enumeration is not a useful benchmark for this instance.

Command:

/home/bernalde/.pixi/bin/pixi run gdplib-benchmark run \
  --instances biofuel \
  --strategies gdp.bigm gdp.hull gdpopt.loa gdpopt.gloa gdpopt.lbb gdpopt.ric \
  --timelimit 300 \
  --solver-profile gams-gurobi \
  --gams-nlp-solver gurobi \
  --gams-mip-solver gurobi \
  --gams-minlp-solver gurobi \
  --gams-local-minlp-solver gurobi \
  --run-id pr133_biofuel_gurobi_300s_20260512 \
  --no-skip-existing

Preflight passed with GAMS available at /home/bernalde/packages/gams51.2_linux_x64_64_sfx/gams, Gurobi passed through as the GAMS solver for all direct and GDPOpt roles, and gdplib.biofuel.build_model() constructed successfully.

Strategy	Termination	Status	Objective / UB	LB	Gap / note	User time
gdp.bigm	feasible	ok	4218.625696814297	2643.195761590122	Gurobi time limit; log gap 37.3446%	300.653s
gdp.hull	optimal	ok	4099.7532529378705	4059.94206212711	Gurobi reports optimal within optcr=0.01; log gap 0.9711%	4.009s
gdpopt.loa	maxTimeLimit	ok	4956.837856441751	-inf / missing	1 GDPOpt iteration; did not converge bounds	302.059s
gdpopt.gloa	maxTimeLimit	ok	4956.837856441751	-inf / missing	1 GDPOpt iteration; did not converge bounds	302.312s
gdpopt.lbb	failed	n/a	n/a	1360.703440658028 before failure	no feasible solution recorded; Pyomo AttributeError: 'GDP_LBB_Solver' object has no attribute '_get_final_results_object'	about 302s
gdpopt.ric	maxTimeLimit	ok	4956.837856441751	-inf / missing	1 GDPOpt iteration; did not converge bounds	302.563s

Generated artifacts are local/ignored under:

• gdplib/biofuel/benchmark_result/pr133_biofuel_gurobi_300s_20260512/
• benchmark_runs/pr133_biofuel_gurobi_300s_20260512/

The solve cases completed, but the benchmark command exited nonzero after writing the results because the optional post-run summary step could not import generate_benchmark_summary_all. The per-case JSON/log files and benchmark_runs/.../results_flat.json were written before that summary failure.

Important caveat: direct transformed GAMS solves on this PR branch still use the benchmark runner's current direct-solve default optcr=0.01; the gdp.hull "optimal" result is therefore a 1% GAMS/Gurobi certificate, not a 1e-6 certificate. The separate benchmark-gap change in #134 should be used for a stricter direct-solve rerun.

Takeaway: GAMS/Gurobi on the direct Hull reformulation is currently the best evidence for this biofuel instance in this run: it found 4099.7532529378705 quickly, with a 4059.94206212711 bound. Direct Big-M and the GDPOpt variants were materially weaker at the 300-second limit.

[bernalde]

Maintainer review of head f4ecc83.

The fix is correct and the regression test is genuine. The base no_modules constraints summed m.num_modules[...] over the entire (site, time) index space, so selecting the inactive or conventional disjunct at any one site forced module variables to zero at all 12 sites — mixed modular/non-modular configurations were infeasible. The site-restricted sums match the docstrings and the symmetric structure of _build_modular_disjunct, and they only remove the unintended cross-site coupling; multi-period module balance, purchase lead time, and salvage logic are untouched. On the base model _module_variable_sites returns all 12 sites, so the new locality assertion is red-on-base — a real regression test.

Findings:

Nonblocking: include fixed variables — tests/test_biofuel.py uses identify_variables(..., include_fixed=False), which silently excludes the num_modules[site, t] entries fixed to 0 during setup periods by _build_modular_disjunct. The assertion holds today only because later periods remain free. The test's meaning (which sites' variables appear in the constraint) does not depend on fixedness — use include_fixed=True so the site set reflects the constraint algebra regardless of build-time fixing.

Nonblocking: shallow transform smoke — the bigm/hull smoke only asserts that no active Disjunction/Disjunct remains, which any structurally valid GDP satisfies. An exception would still fail the test, so this is acceptable, but asserting an expected disaggregated-variable count (hull) would make it catch real hull regressions. Optional.

Nonblocking: closes-vs-refs — the body says "Closes #62", but issue #62's checklist still has Hull, GLOA, Enumeration, and LBB unchecked and the body itself states no solver-backed campaign was run. Merging as-is auto-closes an open tracker; treat the link as Refs #62, or accept the closure explicitly and move the remaining checklist elsewhere.

Summary — Blocking: 0. Nonblocking: 3. Questions: 0. Tests: static verification of the constraint algebra, index sets, and red-on-base reasoning; CI is green at this head (6/6 checks). Merge-ready once the issue-linkage decision is recorded. Posting as COMMENT (author account); the main ruleset requires one approving review from another maintainer.

[bernalde]

Addressed the review of head f4ecc83 in commit 0a9d2d7:

• include-fixed-vars: _module_variable_sites now uses identify_variables(..., include_fixed=True) with a comment explaining why, so the site-locality assertion reflects the constraint algebra regardless of build-time fixing.
• shallow-transform-smoke: the hull leg now also asserts disaggregated variables exist after transformation.
• closes-vs-refs-62: intentionally not a code change — recording the correction here since the body stands: this PR advances but does not complete issue Refactor biofuel to fix benchmark issues #62 (Hull, GLOA, Enumeration, LBB rows remain unverified), so the link should be read as Refs #62. No closing keywords exist in the branch's commit messages, so if the merge should not auto-close Refactor biofuel to fix benchmark issues #62, the auto-closure comes only from the body link; either accept the closure explicitly and move the checklist, or reopen Refactor biofuel to fix benchmark issues #62 after merge.

Tests: pixi run pytest tests/test_biofuel.py — 4 passed. CI for the new head was still starting when this was posted; the previous head was green (6/6).