Fix spectralog benchmark cases - #147
Conversation
|
Benchmark follow-up for I ran the requested 5-minute matrix for this instance: reformulations plus GDPOpt except Run id: Result summary:
The local The suspicious result is GLOA. It certifies I checked the installed Pyomo source in this environment and confirmed it has that exact So the Pyomo fix would not make this PR solve Spectralog globally with GLOA in 5 minutes, but it would make the result conservative: GLOA times out with the correct incumbent and an open gap instead of incorrectly reporting |
bernalde
left a comment
There was a problem hiding this comment.
Maintainer review of head b36cb3f.
The bound derivation is valid: with R identity in the committed data, val[j] = sum_k r_k^2 >= 0, and with P in [0, 1000], A >= 0, the per-k corner maximum gives an upper bound over the whole variable box — a superset of the feasible region — so the bounds cannot cut off any feasible point. The removal of the spectralog,gdpopt.enumerate row from pr58_local.csv is campaign configuration, not committed results, benchmark_cases/ already exists on main, and the cited upstream Pyomo/pyomo#3953 exists and matches the stated cause (2^30 enumeration; 3 compounds x 10 wave numbers = 30 two-disjunct disjunctions checks out). The strengthened test_committed_pr58_local_cases_cover_default_matrix adds exact set equality on top of the count, strictly stronger than base.
Findings:
Nonblocking: bound helper assumptions — _val_bounds silently assumes R is identity and A, C >= 0; the sum-of-squares decomposition breaks if m.R ever gains off-diagonals. Also max_absorbance is loop-invariant in k (hoist it) and misnamed — it bounds the modeled signal sum P*A, not absorbance. Hoist, rename (e.g. max_modeled_signal), and state the identity-R assumption in a comment or assertion.
Nonblocking: vacuous assert — assert not any(... LogicalConstraint ...) in tests/test_spectralog.py is vacuously true: build_model() declares no LogicalConstraint (only BooleanVar + associate_binary_var). Drop the line or comment it as a forward guard; the Disjunction/Disjunct deactivation asserts are the real checks.
Question: optimum discrepancy — the module docstring (untouched) states "Optimal value: 12.0893" while this PR's GLOA run proves LB=UB=15.5406 on this implementation. Since the added bounds provably contain all feasible points, they cannot explain the gap. Is 12.0893 from the source papers' differing formulation/scaling, or is a subsolver bound wrong? Reconcile — either update the docstring with the verified optimum for this implementation (with provenance) or document the formulation difference.
Nonblocking: closes-vs-refs — issue #75's checklist leaves GLOA and Enumeration unchecked; this PR fixes GLOA and excludes Enumeration pending Pyomo/pyomo#3953. "Closes #75" is defensible only if the exclusion is accepted as the resolution; otherwise treat as Refs #75 and track re-adding the enumerate row once the upstream fix lands.
Sibling note: PR #145 also touches tests/test_benchmark.py; the hunks are disjoint and merge-tree reports no conflict.
Summary — Blocking: 0. Nonblocking: 3. Questions: 1. Tests: independent numeric re-derivation of all 8 bound constants (all match to 4 decimals); CI is green at this head (6/6 checks). Merge-ready once the optimum discrepancy and issue-linkage are resolved. Posting as COMMENT (author account); the main ruleset requires one approving review from another maintainer.
|
Addressed the review of head b36cb3f in commit a959a54:
Tests: |
Summary
spectralogobjective-value variables so GDPopt/GLOA gets a bounded relaxation instead of fallbackval[j]bounds.build_model()API intact.spectralog/gdpopt.enumeratefrom the reusable PR58 local benchmark campaign and document why: 30 independent disjunctions imply2^30realizations, and Pyomo currently materializes the enumeration list before enforcingtime_limit.spectralogtests for the derived bounds and Big-M/Hull transformation paths.Tests run
pixi run pytest tests/test_spectralog.py tests/test_benchmark.py::test_committed_pr58_local_cases_cover_default_matrix -v --tb=short-> 4 passed.pixi run gdplib-benchmark run --instances spectralog --strategies gdpopt.gloa --timelimit 60 --solver-profile gams-local --run-id issue75_gloa_fixed --no-summary --no-skip-existing --fail-fast-> 1 row, 0 failures; GDPopt/GLOA optimal in 2 iterations, LB = UB = 15.540610755468409.pixi run pytest tests/test_module_imports.py -v --tb=short-> 74 passed.pixi run pytest tests/test_benchmark.py -v --tb=short-> 21 passed.pixi run lint-black-> passed.pixi run lint-flake8-critical-> passed, 0 critical errors.pixi run lint-flake8-> exit 0; existing style inventory remains.pixi run typos --config ./.github/workflows/typos.toml-> passed.pixi run test-> 338 passed, 1 skipped.git diff --check-> passed.Notes
spectralogwithgdpopt.enumerateto completion. A focused reproduction exceeded the requested 60-second solve limit before reaching a solve result, matching the known Pyomo behavior that enumeration materializes all discrete realizations before honoringtime_limit.Closes #75