Implement UnboundedOrInfeasible termination status#941
Open
rg20 wants to merge 5 commits intoNVIDIA:mainfrom
Open
Implement UnboundedOrInfeasible termination status#941rg20 wants to merge 5 commits intoNVIDIA:mainfrom
rg20 wants to merge 5 commits intoNVIDIA:mainfrom
Conversation
Contributor
Author
|
/ok to test da2b1a1 |
|
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:
📝 WalkthroughWalkthroughAdds a new termination status UnboundedOrInfeasible and propagates it through constants, C++ enums, presolve result types (optionals → concrete result with status and maps), solver handling, Python bindings, tests, and documentation; updates related formatting and string mappings. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/src/mip_heuristics/presolve/third_party_presolve.cpp`:
- Around line 649-656: The early-return that maps Papilo presolve termination
misses papilo::PresolveStatus::kUnbounded: update the conditional that currently
checks kInfeasible || kUnbndOrInfeas to also include
papilo::PresolveStatus::kUnbounded and treat it the same as unbounded by mapping
to third_party_presolve_status_t::UNBNDORINFEAS before constructing the empty
optimization_problem_t and returning third_party_presolve_result_t; apply the
identical change to the matching pattern in apply_pslp() so kUnbounded is
handled the same way there.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2c6c62f9-8539-4496-8cb3-a00035724587
📒 Files selected for processing (13)
cpp/include/cuopt/linear_programming/constants.hcpp/include/cuopt/linear_programming/mip/solver_solution.hppcpp/include/cuopt/linear_programming/pdlp/solver_solution.hppcpp/src/mip_heuristics/presolve/third_party_presolve.cppcpp/src/mip_heuristics/presolve/third_party_presolve.hppcpp/src/mip_heuristics/solve.cucpp/src/mip_heuristics/solver_solution.cucpp/src/pdlp/solve.cucpp/src/pdlp/solver_solution.cucpp/tests/mip/presolve_test.cupython/cuopt/cuopt/linear_programming/solver/solver.pxdpython/cuopt/cuopt/linear_programming/solver/solver_wrapper.pyxpython/cuopt/cuopt/tests/linear_programming/test_lp_solver.py
rg20
requested review from
chris-maes
and removed request for
akifcorduk and
nguidotti
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/cuopt/source/cuopt-c/lp-qp-milp/examples/milp_mps_example.c`:
- Around line 32-33: The code unconditionally calls cuOptGetObjectiveValue and
cuOptGetPrimalSolution even for non-solution termination statuses; update the
example to check the termination status (CUOPT_TERIMINATION_STATUS_OPTIMAL,
CUOPT_TERIMINATION_STATUS_PRIMAL_FEASIBLE, or
CUOPT_TERIMINATION_STATUS_FEASIBLE_FOUND) before invoking cuOptGetObjectiveValue
and cuOptGetPrimalSolution and before printing objective/primal data; leave
other termination branches (e.g.,
CUOPT_TERIMINATION_STATUS_UNBOUNDED_OR_INFEASIBLE) to print a descriptive
message without reading solution values and ensure any existing calls to
cuOptGetObjectiveValue/cuOptGetPrimalSolution are moved inside the guarded
branch.
In `@docs/cuopt/source/cuopt-c/lp-qp-milp/lp-qp-milp-c-api.rst`:
- Line 266: Add explanatory documentation for the new
CUOPT_TERIMINATION_STATUS_UNBOUNDED_OR_INFEASIBLE by supplementing the existing
doxygendefine with a short paragraph (placed above the doxygendefine or in a
nearby "Return codes" / "Termination statuses" section) that states when the
status is returned (e.g., returned by presolvers when the solver determines the
model is either unbounded or infeasible but cannot distinguish which), what that
means for the user’s model, and recommended actions (inspect model formulation,
check bounds/constraints, try presolve diagnostics or relaxation techniques);
ensure the constant name CUOPT_TERIMINATION_STATUS_UNBOUNDED_OR_INFEASIBLE
appears verbatim in the text so it’s clear which status is being described.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e46c94d9-78f5-49b9-89d7-c5f667f1c17b
📒 Files selected for processing (3)
docs/cuopt/source/cuopt-c/lp-qp-milp/examples/milp_mps_example.cdocs/cuopt/source/cuopt-c/lp-qp-milp/examples/simple_qp_example.cdocs/cuopt/source/cuopt-c/lp-qp-milp/lp-qp-milp-c-api.rst
| .. doxygendefine:: CUOPT_TERIMINATION_STATUS_PRIMAL_FEASIBLE | ||
| .. doxygendefine:: CUOPT_TERIMINATION_STATUS_FEASIBLE_FOUND | ||
| .. doxygendefine:: CUOPT_TERIMINATION_STATUS_CONCURRENT_LIMIT | ||
| .. doxygendefine:: CUOPT_TERIMINATION_STATUS_UNBOUNDED_OR_INFEASIBLE |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add explanatory documentation for the new termination status.
The new CUOPT_TERIMINATION_STATUS_UNBOUNDED_OR_INFEASIBLE constant is documented only with a doxygendefine directive, which pulls the basic definition from the source code. However, users need context about when this status is returned and how to interpret it.
Consider adding explanatory text above the constant definition or in a dedicated section that describes:
- When this status is returned (e.g., "Returned by presolvers when the problem is determined to be either unbounded or infeasible, but the specific outcome cannot be distinguished")
- What it means for the user's problem
- Recommended actions (e.g., "Review problem formulation for unboundedness or infeasibility")
As per coding guidelines: "Check if API changes (parameters, return values, errors) are documented. If PR changes public APIs without updating docs, flag as HIGH priority."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/cuopt/source/cuopt-c/lp-qp-milp/lp-qp-milp-c-api.rst` at line 266, Add
explanatory documentation for the new
CUOPT_TERIMINATION_STATUS_UNBOUNDED_OR_INFEASIBLE by supplementing the existing
doxygendefine with a short paragraph (placed above the doxygendefine or in a
nearby "Return codes" / "Termination statuses" section) that states when the
status is returned (e.g., returned by presolvers when the solver determines the
model is either unbounded or infeasible but cannot distinguish which), what that
means for the user’s model, and recommended actions (inspect model formulation,
check bounds/constraints, try presolve diagnostics or relaxation techniques);
ensure the constant name CUOPT_TERIMINATION_STATUS_UNBOUNDED_OR_INFEASIBLE
appears verbatim in the text so it’s clear which status is being described.
Contributor
Author
|
/ok to test b8c02d3 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/cuopt/source/cuopt-c/lp-qp-milp/examples/milp_mps_example.c (1)
101-112:⚠️ Potential issue | 🔴 CriticalGuard output paths when no primal solution is available.
has_primal_solutiononly guards retrieval, but Line 120 still printsobjective_value, and Line 132-Line 135 still dereferencesolution_values. For statuses likeUNBOUNDED_OR_INFEASIBLE, this can print uninitialized data or crash.Proposed fix
@@ - printf("Objective value: %f\n", objective_value); + if (has_primal_solution) { + printf("Objective value: %f\n", objective_value); + } else { + printf("Objective value: N/A\n"); + } @@ - printf("\nSolution: \n"); - for (cuopt_int_t i = 0; i < num_variables; i++) { - printf("x%d = %f\n", i + 1, solution_values[i]); + if (has_primal_solution) { + printf("\nSolution: \n"); + for (cuopt_int_t i = 0; i < num_variables; i++) { + printf("x%d = %f\n", i + 1, solution_values[i]); + } + } else { + printf("\nSolution: N/A for termination status %s (%d)\n", + termination_status_to_string(termination_status), + termination_status); }As per coding guidelines
docs/**/*: Accuracy: Verify code examples compile and run correctly.Also applies to: 123-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/cuopt/source/cuopt-c/lp-qp-milp/examples/milp_mps_example.c` around lines 101 - 112, The example prints and dereferences solution output even when no primal solution exists: wrap any uses of objective_value and solution_values (including the call to cuOptGetObjectiveValue and the call that populates/reads solution_values) behind the has_primal_solution check (the variable computed from termination_status) so that when has_primal_solution is false you skip printing objective_value and accessing solution_values (or print a clear "no primal solution" message); ensure you only call cuOptGetObjectiveValue and the function that gets solution values when has_primal_solution is true and handle/report errors from those calls as currently done.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/src/mip_heuristics/presolve/third_party_presolve.cpp`:
- Around line 479-490: The conversion functions
convert_papilo_presolve_status_to_third_party_presolve_status and
convert_pslp_presolve_status_to_third_party_presolve_status currently have
switches with no default, which can lead to UB if an unexpected enum value
appears; add a default branch to each switch that handles unknown values (e.g.,
assert or log an error) and returns a safe fallback
third_party_presolve_status_t (such as UNCHANGED or a designated UNKNOWN/ERROR
value) so the function always returns a valid enum and does not fall off the
end.
---
Duplicate comments:
In `@docs/cuopt/source/cuopt-c/lp-qp-milp/examples/milp_mps_example.c`:
- Around line 101-112: The example prints and dereferences solution output even
when no primal solution exists: wrap any uses of objective_value and
solution_values (including the call to cuOptGetObjectiveValue and the call that
populates/reads solution_values) behind the has_primal_solution check (the
variable computed from termination_status) so that when has_primal_solution is
false you skip printing objective_value and accessing solution_values (or print
a clear "no primal solution" message); ensure you only call
cuOptGetObjectiveValue and the function that gets solution values when
has_primal_solution is true and handle/report errors from those calls as
currently done.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 589ee1a2-61a7-48ef-bcb0-f9107a028231
📒 Files selected for processing (5)
cpp/src/mip_heuristics/presolve/third_party_presolve.cppcpp/src/mip_heuristics/presolve/third_party_presolve.hppcpp/src/mip_heuristics/solve.cucpp/src/pdlp/solve.cudocs/cuopt/source/cuopt-c/lp-qp-milp/examples/milp_mps_example.c
Contributor
Author
|
/ok to test 165854c |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/src/mip_heuristics/presolve/third_party_presolve.cpp`:
- Around line 612-624: The early-return branches that construct empty
third_party_presolve_result_t (when ctx.status is INFEASIBLE/UNBNDORINFEAS and
the terminal Papilo return around 676-681) do not clear the instance-level
cached mappings/postsolve payloads (pslp_presolver_, pslp_stgs_, and the object
caches exposed by get_reduced_to_original_map()/get_original_to_reduced_map()),
causing stale mappings on reuse; fix by explicitly resetting/clearing
pslp_presolver_, pslp_stgs_ and any internal mapping/postsolve containers (or
calling the presolver's clear/reset method) before returning the empty
third_party_presolve_result_t so mappings and postsolve state are consistent for
subsequent calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ffb9b675-6e8a-411a-b593-3ab6ac0f0bc5
📒 Files selected for processing (1)
cpp/src/mip_heuristics/presolve/third_party_presolve.cpp
|
🔔 Hi @anandhkb, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you! If this is an "epic" issue, then please add the "epic" label to this issue. |
Contributor
Author
|
/ok to test 7eebe2b |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
python/cuopt/cuopt/tests/linear_programming/test_lp_solver.py (1)
747-751: Make this test explicitly presolver-scoped and less brittle.At Line 747, defaults are used; pinning presolve keeps this focused on the presolver regression. At Line 751, compare the enum directly instead of
.namestring matching.♻️ Proposed refinement
def test_unbounded_problem(): problem = Problem("unbounded") x = problem.addVariable(lb=0.0, vtype=CONTINUOUS, name="x") y = problem.addVariable(lb=0.0, vtype=CONTINUOUS, name="y") problem.addConstraint(-1 * x + 2 * y <= 0, name="c1") problem.setObjective(-1 * x - 1 * y, sense=MINIMIZE) settings = SolverSettings() + settings.set_parameter(CUOPT_PRESOLVE, 1) problem.solve(settings) - assert problem.Status.name == "UnboundedOrInfeasible" + assert problem.Status == LPTerminationStatus.UnboundedOrInfeasible🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuopt/cuopt/tests/linear_programming/test_lp_solver.py` around lines 747 - 751, Pin the presolve mode on the SolverSettings instance used in this test and compare the Status enum directly: set settings.presolve = PresolveMode.ON (or the project-specific presolve enum value that enables the presolver) before calling problem.solve(settings), and replace the string comparison assert problem.Status.name == "UnboundedOrInfeasible" with a direct enum comparison assert problem.Status == Status.UnboundedOrInfeasible; ensure the test imports/uses SolverSettings, PresolveMode, and Status so the change is explicit and presolver-scoped.cpp/src/mip_heuristics/presolve/third_party_presolve.cpp (1)
617-620: Consider clearing cached state on early returns for defensive consistency.When returning early due to
INFEASIBLEorUNBNDORINFEASstatus, the instance-level caches (papilo_post_solve_storage_,reduced_to_original_map_,original_to_reduced_map_) are not cleared. If thisthird_party_presolve_tinstance was previously used for a successful presolve, callers accessingget_reduced_to_original_map()orget_original_to_reduced_map()could observe stale mappings from the prior run.While
undo()would not typically be called after an infeasible/unbounded result, clearing the state prevents potential confusion and aligns with the principle that the object's state should reflect the most recentapply()call. Based on learnings: Ensure variables and constraints are accessed from the correct problem context; verify index mapping consistency across problem transformations.Suggested fix
pslp_presolver_ = ctx.presolver; pslp_stgs_ = ctx.settings; + papilo_post_solve_storage_.reset(); + reduced_to_original_map_.clear(); + original_to_reduced_map_.clear(); auto status = convert_pslp_presolve_status_to_third_party_presolve_status(ctx.status); if (ctx.status == PresolveStatus_::INFEASIBLE || ctx.status == PresolveStatus_::UNBNDORINFEAS) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/mip_heuristics/presolve/third_party_presolve.cpp` around lines 617 - 620, Early-return on INFEASIBLE/UNBNDORINFEAS in convert_pslp_presolve_status_to_third_party_presolve_status branch returns a result without clearing instance-level caches, which can leave stale mappings; before constructing and returning the third_party_presolve_result_t in that branch, clear or reset papilo_post_solve_storage_, reduced_to_original_map_, and original_to_reduced_map_ (and any other per-run caches used by apply()/undo()) so the object's state reflects the latest run and callers of get_reduced_to_original_map()/get_original_to_reduced_map() do not see previous mappings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cpp/src/mip_heuristics/presolve/third_party_presolve.cpp`:
- Around line 617-620: Early-return on INFEASIBLE/UNBNDORINFEAS in
convert_pslp_presolve_status_to_third_party_presolve_status branch returns a
result without clearing instance-level caches, which can leave stale mappings;
before constructing and returning the third_party_presolve_result_t in that
branch, clear or reset papilo_post_solve_storage_, reduced_to_original_map_, and
original_to_reduced_map_ (and any other per-run caches used by apply()/undo())
so the object's state reflects the latest run and callers of
get_reduced_to_original_map()/get_original_to_reduced_map() do not see previous
mappings.
In `@python/cuopt/cuopt/tests/linear_programming/test_lp_solver.py`:
- Around line 747-751: Pin the presolve mode on the SolverSettings instance used
in this test and compare the Status enum directly: set settings.presolve =
PresolveMode.ON (or the project-specific presolve enum value that enables the
presolver) before calling problem.solve(settings), and replace the string
comparison assert problem.Status.name == "UnboundedOrInfeasible" with a direct
enum comparison assert problem.Status == Status.UnboundedOrInfeasible; ensure
the test imports/uses SolverSettings, PresolveMode, and Status so the change is
explicit and presolver-scoped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aaed2208-42f9-449e-9d3f-fbb3300e0b4d
📒 Files selected for processing (7)
cpp/include/cuopt/linear_programming/constants.hcpp/src/mip_heuristics/presolve/third_party_presolve.cppcpp/src/mip_heuristics/solver_solution.cucpp/src/pdlp/solve.cucpp/src/pdlp/solver_solution.cudocs/cuopt/source/cuopt-c/lp-qp-milp/lp-qp-milp-c-api.rstpython/cuopt/cuopt/tests/linear_programming/test_lp_solver.py
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/cuopt/source/cuopt-c/lp-qp-milp/lp-qp-milp-c-api.rst
- cpp/include/cuopt/linear_programming/constants.h
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 adds a new termination enum for UnboundedOrInfeasible. This status is returned by presolvers PSLP and Papilo.
Issue
Closes #923
Checklist