Conversation
bernalde
left a comment
There was a problem hiding this comment.
-
Blocking issues: none.
-
Nonblocking issues: none.
-
Questions: none.
-
Tests run and outcomes:
env PYTHONPATH=. /home/bernalde/.pixi/bin/pixi run pytest tests/test_batch_processing.py tests/test_benchmark.py tests/test_release_workflow.py -v --tb=shortResult: 26 passed.
/home/bernalde/.pixi/bin/pixi run testResult: 288 passed, 1 skipped.
/home/bernalde/.pixi/bin/pixi run lintResult: passed. The configured exit-zero flake8 report still prints existing repository style debt.
git diff --check origin/main...HEADResult: passed.
CI is also green for coverage, lint/style-and-typos, pip installation, and Python 3.10/3.11/3.12.
- Merge verdict: this PR is merge-ready from my review. I cannot submit an APPROVE review from this account because it is the PR author, so an eligible reviewer still needs to approve.
6996d42 to
3409cb0
Compare
bernalde
left a comment
There was a problem hiding this comment.
Maintainer review of head 3409cb0.
The cycleTime_log bound derivation itself is mathematically correct (implied bounds; nothing feasible is cut). The problem is that this PR duplicates open PR #132 and additionally carries a broken leftover hunk.
Findings:
Blocking: duplicate import — the root benchmark.py hunk adds DEFAULT_GAMS_OPTCR, to the from gdplib.benchmark import (...) list, but main already imports that name on the immediately preceding line, so the diff introduces a literal duplicate in the same import statement. Nothing in this PR needs to touch this re-export shim; the hunk is residue from the branch's earlier optcr work (fix/benchmark-gams-optcr-1e-6). Fix: drop the benchmark.py hunk entirely.
Blocking: superseded by #132 — gdplib/batch_processing/batch_processing.py is byte-identical to PR #132's fix except for docstring wording and one blank line, and #132's tests/test_batch_processing.py is a strict superset of this one (this rewrite drops the FBBT regression test, the Disjunct deactivation assertion, and the opt-in GAMS/BARON GLOA smoke). The bound test here recomputes the expected values with the same formula as the implementation, so it pins regressions but cannot catch a wrong derivation. Recommended resolution: merge #132 and close this PR as superseded — its only unique content is the broken import hunk above.
Nonblocking: stale body — the PR description claims optcr=1e-6 plumbing, README/AGENTS documentation, and benchmark-metadata changes that do not exist in this three-file diff. If this PR survives, the description needs to be rewritten to match the actual change.
Also note both this PR and #132 say "Closes #61"; whichever lands first auto-closes the tracker, and the other must be closed manually with a cross-reference.
Summary — Blocking: 2. Nonblocking: 1. Questions: 0. Tests: static verification of the bound math and a diff of this head against PR #132's head. I would not merge this until the blocking issues above are addressed; the cleaner path is closing this PR in favor of #132. Posting as COMMENT (author account); the main ruleset requires one approving review from another maintainer.
|
Address pass for the review of head 3409cb0 — intentionally no code pushed:
Recommended human action: close this PR as superseded by #132 (both bodies say |
Summary
batch_processing.cycleTime_logfrom the production horizon and processing-time constraints.option optcr=1e-6consistently for direct transformed solves and GDPopt GAMS role solves.Note: this branch addresses the
batch_processingbenchmark issue, which is #61. Issue #62 is the separatebiofuelissue, so this PR intentionally does not close #62.Solver Evidence
Direct transformed verification:
Result: 6 rows, 0 failures. BARON/DICOPT objectives were unchanged to numerical tolerance:
679365.334866, optimal.679365.323287, optimal.679365.334869, optimal.679365.334507, optimal.GDPopt GLOA verification:
Result: 1 row, 0 failures. GLOA now reaches
maxTimeLimitinstead of failing in Pyomo FBBT. Removing only the newcycleTime_logbounds reproduced the old warning andAssertionErrorpath.Tests Run
Result: 26 passed.
/home/bernalde/.pixi/bin/pixi run testResult: 288 passed, 1 skipped.
Result: passed. The configured exit-zero flake8 report still prints existing repository style debt.
Result: passed.
Notes
The direct targeted command without
PYTHONPATH=.:failed during collection because
tests/test_benchmark.pyimports the repo-rootbenchmark.py; the full documentedpixi run testpath passes.Closes #61.