Implement UnboundedOrInfeasible termination status#941
Open
rg20 wants to merge 4 commits intoNVIDIA:mainfrom
Open
Implement UnboundedOrInfeasible termination status#941rg20 wants to merge 4 commits intoNVIDIA:mainfrom
rg20 wants to merge 4 commits intoNVIDIA:mainfrom
Conversation
Contributor
Author
|
/ok to test da2b1a1 |
📝 WalkthroughWalkthroughAdds a new termination status UnboundedOrInfeasible and threads it through constants, C++ enums, presolve result types (replacing optionals with concrete results including status and maps), solver handling, Python bindings, tests, and documentation. 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)
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: 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
Comment on lines
612
to
+624
| pslp_presolver_ = ctx.presolver; | ||
| pslp_stgs_ = ctx.settings; | ||
|
|
||
| auto status = convert_pslp_presolve_status_to_third_party_presolve_status(ctx.status); | ||
| if (ctx.status == PresolveStatus_::INFEASIBLE || ctx.status == PresolveStatus_::UNBNDORINFEAS) { | ||
| return std::nullopt; | ||
| optimization_problem_t<i_t, f_t> empty_problem(op_problem.get_handle_ptr()); | ||
| return third_party_presolve_result_t<i_t, f_t>{status, std::move(empty_problem), {}, {}, {}}; | ||
| } | ||
|
|
||
| auto opt_problem = build_optimization_problem_from_pslp<i_t, f_t>( | ||
| pslp_presolver_, op_problem.get_handle_ptr(), maximize_, original_obj_offset); | ||
|
|
||
| return std::make_optional(third_party_presolve_result_t<i_t, f_t>{opt_problem, {}}); | ||
| return third_party_presolve_result_t<i_t, f_t>{status, std::move(opt_problem), {}, {}, {}}; |
There was a problem hiding this comment.
Reset cached mapping/postsolve state on PSLP and terminal Papilo returns.
These branches return empty maps in third_party_presolve_result_t, but the object-level caches exposed by get_reduced_to_original_map() / get_original_to_reduced_map() in cpp/src/mip_heuristics/presolve/third_party_presolve.hpp:79-80 are left untouched. If the same third_party_presolve_t instance is reused after a prior Papilo solve, callers can observe stale index mappings, and the Papilo terminal branch also keeps the previous postsolve payload alive for undo().
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) {
optimization_problem_t<i_t, f_t> empty_problem(op_problem.get_handle_ptr());
return third_party_presolve_result_t<i_t, f_t>{status, std::move(empty_problem), {}, {}, {}};
@@
auto status = convert_papilo_presolve_status_to_third_party_presolve_status(result.status);
if (result.status == papilo::PresolveStatus::kInfeasible ||
result.status == papilo::PresolveStatus::kUnbndOrInfeas ||
result.status == papilo::PresolveStatus::kUnbounded) {
+ papilo_post_solve_storage_.reset();
+ reduced_to_original_map_.clear();
+ original_to_reduced_map_.clear();
optimization_problem_t<i_t, f_t> empty_problem(op_problem.get_handle_ptr());
return third_party_presolve_result_t<i_t, f_t>{status, std::move(empty_problem), {}, {}, {}};
}Based on learnings: Ensure variables and constraints are accessed from the correct problem context (original vs presolve vs folded vs postsolve); verify index mapping consistency across problem transformations.
Also applies to: 676-681
🤖 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 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.
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