add mip start attribute#1305
Conversation
|
/ok to test 9d41467 |
📝 WalkthroughWalkthroughThis PR extends the Python Linear Programming API with MIP warm-start support. The ChangesMIP Warm-Start Hints
🎯 2 (Simple) | ⏱️ ~8 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@python/cuopt/cuopt/linear_programming/problem.py`:
- Around line 206-218: The new public methods setMIPStart and getMIPStart lack
type hints and complete docstrings; add type annotations (def setMIPStart(self,
val: float) -> None and def getMIPStart(self) -> float), update their docstrings
to include Parameters, Returns, and Raises sections, and validate input in
setMIPStart (attempt float(val) and raise a TypeError with a clear message if
conversion fails) while documenting that float("nan") unsets the value and that
getMIPStart returns a float (NaN when unset); reference the instance attribute
MIPStart in the docstrings so callers know the backing field.
- Around line 1487-1488: The current check forwards self.mip_start if any entry
is non-NaN, allowing partially-NaN vectors; change the guard to only forward
when all entries are finite (no NaN/Inf) — e.g., replace the condition on
self.mip_start with a check using np.all(np.isfinite(self.mip_start)) while
still checking size > 0, and only call
dm.set_initial_primal_solution(self.mip_start) when that holds; reference
self.mip_start and dm.set_initial_primal_solution to locate the change.
In `@python/cuopt/cuopt/tests/linear_programming/test_python_API.py`:
- Around line 485-524: Add an additional edge-case assertion in test_mip_start
to exercise the "all-NaN" branch of Problem._to_data_model: create a scenario
where x.MIPStart and y.MIPStart remain NaN (do not call setMIPStart or assign),
call prob._to_data_model(), then call prob.model.get_initial_primal_solution()
and assert that no initial primal solution is set (e.g., assert the returned
value is None or an empty sequence / is falsy). Reference Variable.MIPStart,
test_mip_start, _to_data_model, and get_initial_primal_solution when locating
where to add this check.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8b3517e2-e0d6-4e94-ab72-0f80cdbd9c42
📒 Files selected for processing (2)
python/cuopt/cuopt/linear_programming/problem.pypython/cuopt/cuopt/tests/linear_programming/test_python_API.py
| def setMIPStart(self, val): | ||
| """ | ||
| Sets the MIP start (initial primal solution hint) value for the | ||
| variable. Use ``float("nan")`` to unset. | ||
| """ | ||
| self.MIPStart = float(val) | ||
|
|
||
| def getMIPStart(self): | ||
| """ | ||
| Returns the MIP start (initial primal solution hint) value of the | ||
| variable. Defaults to NaN when unset. | ||
| """ | ||
| return self.MIPStart |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add type hints and complete API docstrings for new public methods.
setMIPStart/getMIPStart are new public APIs but currently lack type annotations and full docstring sections (Parameters, Returns, Raises).
Suggested patch
- def setMIPStart(self, val):
+ def setMIPStart(self, val: float) -> None:
"""
- Sets the MIP start (initial primal solution hint) value for the
- variable. Use ``float("nan")`` to unset.
+ Set the MIP start (initial primal solution hint) value.
+
+ Parameters
+ ----------
+ val : float
+ Initial value for this variable. Use ``float("nan")`` to unset.
+
+ Raises
+ ------
+ ValueError
+ If ``val`` cannot be converted to float.
"""
- self.MIPStart = float(val)
+ try:
+ self.MIPStart = float(val)
+ except (TypeError, ValueError) as e:
+ raise ValueError("MIPStart must be a numeric value or NaN") from e
- def getMIPStart(self):
+ def getMIPStart(self) -> float:
"""
- Returns the MIP start (initial primal solution hint) value of the
- variable. Defaults to NaN when unset.
+ Get the MIP start (initial primal solution hint) value.
+
+ Returns
+ -------
+ float
+ The current hint value, or ``NaN`` when unset.
"""
return self.MIPStartAs per coding guidelines, “Type hints on NEW public functions/classes” and “Docstring CONTENT on new public APIs — params, returns, raises”.
🤖 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 `@python/cuopt/cuopt/linear_programming/problem.py` around lines 206 - 218, The
new public methods setMIPStart and getMIPStart lack type hints and complete
docstrings; add type annotations (def setMIPStart(self, val: float) -> None and
def getMIPStart(self) -> float), update their docstrings to include Parameters,
Returns, and Raises sections, and validate input in setMIPStart (attempt
float(val) and raise a TypeError with a clear message if conversion fails) while
documenting that float("nan") unsets the value and that getMIPStart returns a
float (NaN when unset); reference the instance attribute MIPStart in the
docstrings so callers know the backing field.
| if self.mip_start.size > 0 and not np.all(np.isnan(self.mip_start)): | ||
| dm.set_initial_primal_solution(self.mip_start) |
There was a problem hiding this comment.
Guard against forwarding partially-NaN MIP starts to the data model.
Current condition forwards self.mip_start when any entry is non-NaN, so vectors like [5.0, NaN, ...] are passed through. That can inject invalid numeric values into solver initialization.
Suggested patch
- if self.mip_start.size > 0 and not np.all(np.isnan(self.mip_start)):
- dm.set_initial_primal_solution(self.mip_start)
+ if self.mip_start.size > 0 and not np.all(np.isnan(self.mip_start)):
+ if np.any(np.isnan(self.mip_start)):
+ raise ValueError(
+ "MIPStart must be provided for all variables or left unset for all."
+ )
+ dm.set_initial_primal_solution(self.mip_start)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if self.mip_start.size > 0 and not np.all(np.isnan(self.mip_start)): | |
| dm.set_initial_primal_solution(self.mip_start) | |
| if self.mip_start.size > 0 and not np.all(np.isnan(self.mip_start)): | |
| if np.any(np.isnan(self.mip_start)): | |
| raise ValueError( | |
| "MIPStart must be provided for all variables or left unset for all." | |
| ) | |
| dm.set_initial_primal_solution(self.mip_start) |
🤖 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 `@python/cuopt/cuopt/linear_programming/problem.py` around lines 1487 - 1488,
The current check forwards self.mip_start if any entry is non-NaN, allowing
partially-NaN vectors; change the guard to only forward when all entries are
finite (no NaN/Inf) — e.g., replace the condition on self.mip_start with a check
using np.all(np.isfinite(self.mip_start)) while still checking size > 0, and
only call dm.set_initial_primal_solution(self.mip_start) when that holds;
reference self.mip_start and dm.set_initial_primal_solution to locate the
change.
| def test_mip_start(): | ||
| # Build a small MIP and feed it an (already-optimal) MIP start through | ||
| # the new Variable.MIPStart attribute. We verify: | ||
| # - the attribute defaults to NaN | ||
| # - setMIPStart / direct assignment both work | ||
| # - the values reach the data model via set_initial_primal_solution | ||
| # - solving still produces the optimal result | ||
| # - leaving every MIPStart as NaN does not set an initial primal sol | ||
| prob = Problem("MIP_start") | ||
| x = prob.addVariable(lb=0, ub=10, vtype=INTEGER, name="x") | ||
| y = prob.addVariable(lb=0, ub=10, vtype=INTEGER, name="y") | ||
|
|
||
| assert math.isnan(x.getMIPStart()) | ||
| assert math.isnan(y.MIPStart) | ||
|
|
||
| prob.addConstraint(x + y <= 10, name="c1") | ||
| prob.addConstraint(x - y >= 0, name="c2") | ||
| prob.setObjective(x + 2 * y, sense=MAXIMIZE) | ||
|
|
||
| x.setMIPStart(5) | ||
| y.MIPStart = 5.0 | ||
|
|
||
| assert x.getMIPStart() == 5.0 | ||
| assert y.getMIPStart() == 5.0 | ||
|
|
||
| prob._to_data_model() | ||
| initial_primal = prob.model.get_initial_primal_solution() | ||
| assert len(initial_primal) == 2 | ||
| assert initial_primal[0] == pytest.approx(5.0) | ||
| assert initial_primal[1] == pytest.approx(5.0) | ||
|
|
||
| settings = SolverSettings() | ||
| settings.set_parameter("time_limit", 5) | ||
| prob.solve(settings) | ||
|
|
||
| assert prob.Status.name == "Optimal" | ||
| assert prob.ObjValue == pytest.approx(15) | ||
| assert x.Value == pytest.approx(5) | ||
| assert y.Value == pytest.approx(5) | ||
|
|
There was a problem hiding this comment.
Add edge-case assertion for the all-NaN branch.
This test validates the “set hint” path, but not the “all hints unset” path introduced by the new branch in _to_data_model. Please add a case where all MIPStart remain NaN and assert no initial primal solution is set.
As per coding guidelines, “Flag missing coverage for edge cases (empty, infeasible, unbounded, degenerate) when adding new code paths” and for python/**/tests/**, focus on edge cases.
🤖 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 `@python/cuopt/cuopt/tests/linear_programming/test_python_API.py` around lines
485 - 524, Add an additional edge-case assertion in test_mip_start to exercise
the "all-NaN" branch of Problem._to_data_model: create a scenario where
x.MIPStart and y.MIPStart remain NaN (do not call setMIPStart or assign), call
prob._to_data_model(), then call prob.model.get_initial_primal_solution() and
assert that no initial primal solution is set (e.g., assert the returned value
is None or an empty sequence / is falsy). Reference Variable.MIPStart,
test_mip_start, _to_data_model, and get_initial_primal_solution when locating
where to add this check.
rgsl888prabhu
left a comment
There was a problem hiding this comment.
@Iroy30 The changes look good, just had a suggestion, if we have a doc example for warmstart, may be we can update it with this example so it encompasses.
|
/merge |
Description
This PR adds a per-variable attribute
MIPStartthat a user can set to specify the initial variable(primal) values for the problem.Issue
Checklist