Replace Solver Status API#27
Merged
Merged
Conversation
Replace `SolverStatus` with two enums: `TerminationStatus` (why the solver stopped) and `PrimalStatus` (whether a usable point came back)
Assisted-by: GitHub Copilot
Assisted-by: GitHub Copilot
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe solver API now separates termination reasons from primal-point availability. Backend translators populate ChangesTermination and primal status split
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
crates/oximo-solver/src/status.rs (1)
32-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct contract tests for the new status helpers.
admits_primal()andPrimalStatus::infer()now define the shared incumbent/solution semantics for every translator, but this module doesn't pin the key combinations down yet. A small table-driven test overOptimal,LocallyOptimal, the limit states, andNotSolvedwould make future backend mapping changes much safer.Also applies to: 64-82
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/oximo-solver/src/status.rs` around lines 32 - 48, Add direct table-driven contract tests in the status module to lock down the shared semantics between TerminationStatus::admits_primal() and PrimalStatus::infer(). Cover the key combinations mentioned in the review, including Optimal, LocallyOptimal, the various limit states, and NotSolved, and assert the expected primal-availability/inferral behavior so translator mapping changes are caught early. Use the existing TerminationStatus and PrimalStatus symbols in this module to place the tests alongside the helper implementations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/oximo-baron/README.md`:
- Around line 54-56: The README’s `## Result` section is still documenting the
old `status`-based API, while the example above already uses
`result.termination`, `result.primal_status`, and `result.objective()`. Update
the later result-field list to match the renamed fields and new success model,
and remove any remaining references to the removed `status` field so the
`Result` documentation is consistent with the current API.
In `@crates/oximo-baron/src/translate.rs`:
- Line 563: The `PrimalStatus::infer` call in `translate` is using
`!solutions.is_empty()` as a proxy for a usable primal point, but the `nodeopt
== -3` fallback can add a `SolutionPoint` with an empty `primal` map. Update the
status inference logic so it only treats entries in `solutions` as usable when
they actually contain primal data, and ensure the
`SolutionPoint`/`has_solution()` path reflects that distinction. Also tighten
the existing `nodeopt == -3` regression test to assert that this fallback does
not report a usable solution via `has_solution()`, `primal()`, or `value_of()`.
In `@crates/oximo-gams/README.md`:
- Around line 58-60: Update the README’s later ## Result documentation to match
the new split result API shown in the example, replacing references to the
removed status field and pre-split contract with the current result fields used
by Result::termination, Result::primal_status, and Result::objective(). Keep the
terminology consistent throughout the document so the Result section and the
example describe the same contract.
In `@crates/oximo-gurobi/src/translate.rs`:
- Around line 79-90: Do not use TerminationStatus alone to imply a primal
solution in translate.rs: the collect_solution/single-point path in translate
should only mark a feasible primal when Gurobi actually has an incumbent. Update
collect_solution and the call site that computes primal_status to gate on
SolCount/X availability (and not just admits_primal or limit/interrupted
termination), so has_solution() stays false when no real solution exists.
In `@crates/oximo/README.md`:
- Around line 267-272: The README example in the `match result.termination`
snippet should not assume `result.objective()` is always present for
`TerminationStatus::Optimal`; update the example to avoid unwrapping the
objective and instead handle the optional value safely, since
`Result::objective` can be absent even when a feasible solution exists. Keep the
example aligned with the `TerminationStatus` handling shown in
`result.termination` and use a pattern that prints the objective only when it is
available.
In `@README.md`:
- Around line 267-272: The README example still implies that
TerminationStatus::Optimal alone guarantees an objective is available, which
conflicts with the new split between termination and solution presence. Update
the match on result.termination so the Optimal arm also guards any call to
result.objective() with result.has_solution() (or the equivalent primal_status
check), and adjust the sample text to show that objective reads are
solution-dependent rather than tied to termination alone. Keep the TimeLimit arm
consistent with this pattern and reference the result.has_solution() /
result.objective() APIs in the example narrative.
---
Nitpick comments:
In `@crates/oximo-solver/src/status.rs`:
- Around line 32-48: Add direct table-driven contract tests in the status module
to lock down the shared semantics between TerminationStatus::admits_primal() and
PrimalStatus::infer(). Cover the key combinations mentioned in the review,
including Optimal, LocallyOptimal, the various limit states, and NotSolved, and
assert the expected primal-availability/inferral behavior so translator mapping
changes are caught early. Use the existing TerminationStatus and PrimalStatus
symbols in this module to place the tests alongside the helper implementations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dc4bae8d-c465-4fb6-8d2a-6b10219c45c2
📒 Files selected for processing (25)
README.mdcrates/oximo-baron/README.mdcrates/oximo-baron/src/translate.rscrates/oximo-gams/README.mdcrates/oximo-gams/src/translate.rscrates/oximo-gurobi/README.mdcrates/oximo-gurobi/src/translate.rscrates/oximo-gurobi/tests/nonlinear.rscrates/oximo-highs/README.mdcrates/oximo-highs/src/translate.rscrates/oximo-solver/README.mdcrates/oximo-solver/src/lib.rscrates/oximo-solver/src/result.rscrates/oximo-solver/src/status.rscrates/oximo/README.mdcrates/oximo/examples/baron_robot.rscrates/oximo/examples/lot_sizing.rscrates/oximo/examples/parametric_pricing.rscrates/oximo/examples/process_selection.rscrates/oximo/examples/reaction_path.rscrates/oximo/src/lib.rscrates/oximo/tests/solve.rscrates/oximo/tests/solve_baron.rscrates/oximo/tests/solve_gams.rscrates/oximo/tests/solve_gurobi.rs
Assisted-by: GitHub Copilot
Assisted-by: GitHub Copilot
Assisted-by: GitHub Copilot
GermanHeim
added a commit
that referenced
this pull request
Jun 27, 2026
Since #27 is a breaking change, we can already remove the deprecated code.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR replaces the
SolverStatuswith two enums:TerminationStatus(why the solver stopped) andPrimalStatus(whether a usable point came back).This is a breaking change.
Checklist
cargo fmtandcargo clippyto ensure code qualitycargo test)cargo test --features gamspasses (requires GAMS)cargo test --features gurobipasses (requires Gurobi)cargo test --features baronpasses (requires BARON)Summary by CodeRabbit
termination(why the solve stopped) andprimal_status(usable solution availability), plushas_solution().best_boundandgapwhen finite bounds are available.termination/primal_statusinstead of the prior status field.