Add algebraic __str__ and detailed __repr__ to Python LP API classes - #1400
Add algebraic __str__ and detailed __repr__ to Python LP API classes#1400jackthepunished wants to merge 3 commits into
Conversation
Adds __str__ and __repr__ to Variable, LinearExpression, QuadraticExpression, Constraint, and Problem. Printing these objects now shows their algebraic form (e.g. '2.0 * x + 3.0 * y <= 10.0') and the REPL shows a detailed summary, improving debuggability in notebooks and interactive sessions. The change is purely additive. Signed-off-by: jackthepunished <kosapinarbahadir@gmail.com>
📝 WalkthroughWalkthroughThis PR adds readable ChangesHuman-readable display support for LP API objects
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/cuopt/cuopt/tests/linear_programming/test_python_API.py (1)
14-25:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd missing
LinearExpressionimport to preventNameErrorin the new test.
LinearExpressionis referenced at Line 873, Line 875, and Line 876 but is not imported, so this test will fail at runtime.As per coding guidelines, “Run pre-commit hooks before committing code to enforce code linters and formatters”; Ruff’s F821 here indicates a correctness break that should be fixed before merge.Proposed fix
from cuopt.linear_programming.problem import ( CONTINUOUS, INTEGER, MAXIMIZE, MINIMIZE, SEMI_CONTINUOUS, CType, + LinearExpression, Problem, VType, sense, QuadraticExpression, )🤖 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 14 - 25, The test imports from cuopt.linear_programming.problem but omits LinearExpression, causing NameError in tests referencing LinearExpression; update the import list in test_python_API.py to include LinearExpression (i.e., add LinearExpression to the grouped import alongside CONTINUOUS, INTEGER, MAXIMIZE, MINIMIZE, SEMI_CONTINUOUS, CType, Problem, VType, sense, QuadraticExpression) so the symbol is available where referenced.Sources: Coding guidelines, Linters/SAST tools
🤖 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`:
- Line 21: The _SENSE_SYMBOLS dictionary is created before the constants LE, GE,
and EQ are defined, causing an import-time NameError; fix by deferring its
construction until after those constants are declared (or by keying it with the
raw numeric/char codes used for LE/GE/EQ instead of the names), i.e., move or
rebuild _SENSE_SYMBOLS after the definitions of LE, GE, EQ (or replace keys with
the literal codes) so references in _SENSE_SYMBOLS resolve correctly.
---
Outside diff comments:
In `@python/cuopt/cuopt/tests/linear_programming/test_python_API.py`:
- Around line 14-25: The test imports from cuopt.linear_programming.problem but
omits LinearExpression, causing NameError in tests referencing LinearExpression;
update the import list in test_python_API.py to include LinearExpression (i.e.,
add LinearExpression to the grouped import alongside CONTINUOUS, INTEGER,
MAXIMIZE, MINIMIZE, SEMI_CONTINUOUS, CType, Problem, VType, sense,
QuadraticExpression) so the symbol is available where referenced.
🪄 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: 84099242-4510-46e2-b7dc-41322ef3afa6
📒 Files selected for processing (2)
python/cuopt/cuopt/linear_programming/problem.pypython/cuopt/cuopt/tests/linear_programming/test_python_API.py
|
This is interesting. Could you provide some example output, showing what strings this would compute, on a few different problems? |
|
Here are some examples of what you'd see when printing or repr'ing objects after this change. On a small MILP with constraints like 2x + 3y <= 10, x - y >= 0, and x + 1 == 5, you get readable algebra instead of memory addresses: str(c1) gives 2.0 * x + 3.0 * y <= 10.0, str(c2) gives x - y >= 0.0, and str(c3) gives x + 1.0 == 5.0 (the last one keeps the constant on the LHS as the user wrote it, not the normalized internal form). Variables print as their name (str(x) → x) or as C{index} when unnamed (str(z) → C2), and repr(x) gives something like <cuopt.Variable 'x' (index=0), type=CONTINUOUS, bounds=[0.0, 10.0], value=nan>. For a QP, quadratic terms format naturally: str(xx) → x^2, str(xx + 2xy + 3x) → x^2 + 2.0 * x * y + 3.0 * x, and mixed-sign expressions like -xx + 0.5yy + x*y → -x^2 + 0.5 * y^2 + x * y. On the problem itself, str(prob) is a short summary with the problem name, objective sense, variable/constraint counts, non-zero count, and after solve the status and objective value, while repr(prob) stays compact, e.g. <cuopt.Problem 'str_repr_test' (3 vars, 3 constrs, IsMIP=True)>. |
|
@chris-maes can you /review |
|
@chris-maes May I get your review on this PR |
|
I think this would be a nice addition. My main concern on the behavior side is there's no truncation in the case of large expressions. I doubt users want to see a linear or quadratic expression with 10,000 terms in it. If we can work out the right behavior in this case and validate it in the both REPL and notebooks then we'll be on track to merging this. |
mlubin
left a comment
mlubin
left a comment
There was a problem hiding this comment.
Requesting the changes I mentioned above.
Linear and quadratic expressions now render only the first
_MAX_DISPLAY_TERMS (10) terms followed by a "... (N more terms)" marker,
so printing a model with thousands of terms stays readable in a REPL or
notebook instead of flooding the output. Applies to both __str__ and
__repr__ (and therefore Constraint, whose LHS is the expression); the
cap is a module constant and can be set to None to disable.
Also fix an import-time NameError: the module-level _SENSE_SYMBOLS table
referenced LE/GE/EQ before they were defined. Re-key it by the
underlying CType char codes ("L"/"G"/"E"), which CType members compare
and hash equal to, so the module imports regardless of definition order.
Add test_str_truncation_large_expression covering the linear/quadratic
head + marker, the exactly-at-cap (no marker) and one-over (singular)
boundaries, and the truncation-disabled case.
Signed-off-by: jackthepunished <kosapinarbahadir@gmail.com>
aca0220 to
cd6c5ca
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tests/linear_programming/test_python_API.py`:
- Around line 991-998: The quadratic truncation test is too loose because it
only asserts an upper bound on the rendered term count, so regressions that
display fewer than the intended display limit could still pass. Tighten the
assertions in the QuadraticExpression string/representation test to match the
same exact truncation boundary used by the linear expression branch, using
qexpr/qs and the existing _MAX_DISPLAY_TERMS behavior to verify the precise
number of displayed quadratic terms and the expected ellipsis suffix.
🪄 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: 99a0419f-4f28-4154-833f-85c8de41d7dc
📒 Files selected for processing (2)
python/cuopt/cuopt/linear_programming/problem.pypython/cuopt/cuopt/tests/linear_programming/test_python_API.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/cuopt/cuopt/linear_programming/problem.py
mlubin
added
non-breaking
| name = var.VariableName | ||
| if name: | ||
| return name | ||
| if getattr(var, "index", -1) >= 0: |
There was a problem hiding this comment.
Why getattr instead of var.getVariableIndex()? When would the attribute not exist? And when would it be -1?
| @@ -16,6 +16,133 @@ | |||
| import warnings | |||
|
|
|||
|
|
|||
| # ---- Display helpers for __str__/__repr__ ---- | |||
There was a problem hiding this comment.
The location of this code isn't very natural. Why are we defining the printing helpers before VType, CType, Variable, etc?
| # string. Using the codes (rather than the LE/GE/EQ aliases) also keeps this | ||
| # module-level table independent of definition order. | ||
| _SENSE_SYMBOLS = {"L": "<=", "G": ">=", "E": "=="} | ||
| _TYPE_NAMES = {"C": "CONTINUOUS", "I": "INTEGER", "S": "SEMI_CONTINUOUS"} |
There was a problem hiding this comment.
Use VType().name rather than hardcoding this mapping.
| # Keyed by the underlying CType char codes ("L"/"G"/"E"). CType is a | ||
| # ``(str, Enum)`` whose members compare and hash equal to these codes, so the | ||
| # lookup works whether ``Constraint.Sense`` holds a CType member or a raw | ||
| # string. Using the codes (rather than the LE/GE/EQ aliases) also keeps this |
There was a problem hiding this comment.
Could this mapping be a member of the CType enum?
|
|
||
| def __repr__(self): | ||
| name = _var_display_name(self) | ||
| idx = getattr(self, "index", -1) |
| @@ -1322,6 +1492,7 @@ def __init__(self, expr, sense, rhs, name=""): | |||
| self.ConstraintName = name | |||
| self.DualValue = float("nan") | |||
| self.Slack = float("nan") | |||
| self._expr = expr | |||
There was a problem hiding this comment.
Seems like we're potentially doubling the model's memory usage by storing an extra copy of the constraint data here. That's not ideal.
|
🔔 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. |
1 similar comment
|
🔔 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. |
|
🔔 Hi @anandhkb @mlubin, 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. |
|
@jackthepunished do you plan to follow up on this PR? |
|
Firstly I'm sorry to leave this PR without notice, but I was a companion for a family member at hospital so I couldn't give any attention to here, i'll get onto it asap. |
- Replace _SENSE_SYMBOLS with a CType.symbol property and _TYPE_NAMES with VType(...).name lookups. - Fold _var_display_name into Variable.__str__ and move the remaining display helpers (_MAX_DISPLAY_TERMS, _ExprBuilder, _format_linear) next to the expression classes that use them. - Use direct .index access instead of getattr; the attribute is always set in Variable.__init__ (-1 until addVariable assigns it). - Stop storing the expression on Constraint; __str__ now renders lazily from the solver data the constraint already holds, so constraints print in normalized form (duplicates merged, constants folded into the RHS) and stay in sync after updateConstraint. Quadratic rows now record all participating variables so QCMATRIX indices map to names. - Tests: add missing LinearExpression import, tighten the quadratic truncation assertion, and cover quadratic/duplicate/updateConstraint constraint display.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 51-55: Update the CType.symbol property in
python/cuopt/cuopt/linear_programming/problem.py at lines 51-55 to add a str
return annotation and document that it returns the algebraic constraint symbol;
apply the same annotation/documentation change to the sibling display methods at
lines 343-359 and 1559-1579 as requested.
🪄 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: 07d9fb5e-241f-42f7-bfc6-66c3ff95c680
📒 Files selected for processing (2)
python/cuopt/cuopt/linear_programming/problem.pypython/cuopt/cuopt/tests/linear_programming/test_python_API.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/cuopt/cuopt/tests/linear_programming/test_python_API.py
| @property | ||
| def symbol(self): | ||
| """Algebraic symbol used when printing constraints.""" | ||
| return {CType.LE: "<=", CType.GE: ">=", CType.EQ: "=="}[self] | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -A4 -B2 \
'def symbol|def __str__|def __repr__' \
python/cuopt/cuopt/linear_programming/problem.pyRepository: NVIDIA/cuopt
Length of output: 2928
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=python/cuopt/cuopt/linear_programming/problem.py
printf '%s\n' '--- targeted diff ---'
git diff --unified=12 -- "$file" | sed -n \
'/CType/,/^[^+ -].*$/p' | head -n 220
printf '%s\n' '--- class and method definitions ---'
rg -n '^(class (CType|Variable|Constraint)| def (symbol|__str__|__repr__))' "$file"
printf '%s\n' '--- surrounding source ---'
sed -n '35,65p;315,375p;1535,1590p' "$file"Repository: NVIDIA/cuopt
Length of output: 5668
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast
from pathlib import Path
path = Path("python/cuopt/cuopt/linear_programming/problem.py")
tree = ast.parse(path.read_text())
wanted = {"CType", "Variable", "Constraint"}
for node in tree.body:
if isinstance(node, ast.ClassDef) and node.name in wanted:
print(f"class {node.name}:")
for item in node.body:
if isinstance(item, (ast.FunctionDef, ast.AsyncFunctionDef)):
if item.name in {"symbol", "__str__", "__repr__"}:
returns = ast.unparse(item.returns) if item.returns else "<missing>"
doc = ast.get_docstring(item, clean=False)
print(
f" {item.name}: return={returns}; "
f"docstring={'yes' if doc else 'no'}"
)
if doc:
print(f" {doc!r}")
PYRepository: NVIDIA/cuopt
Length of output: 415
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=python/cuopt/cuopt/linear_programming/problem.py
printf '%s\n' '--- repository state ---'
git status --short
git log -1 --oneline
git diff --stat
git show --stat --oneline HEAD
printf '%s\n' '--- commit-level changes in target file ---'
git show --format= --unified=3 HEAD -- "$file" | sed -n '1,260p'
printf '%s\n' '--- method blocks and immediate docstrings ---'
python3 - "$file" <<'PY'
from pathlib import Path
import re
import sys
text = Path(sys.argv[1]).read_text().splitlines()
targets = {"symbol", "__str__", "__repr__"}
class_name = None
for i, line in enumerate(text):
class_match = re.match(r"class\s+([A-Za-z_]\w*)\b", line)
if class_match:
class_name = class_match.group(1)
method_match = re.match(r" def\s+([A-Za-z_]\w*)\s*\((.*?)\)\s*(?:->\s*([^:]+))?:", line)
if method_match and method_match.group(1) in targets:
name, args, ret = method_match.groups()
print(f"{class_name}.{name}: return={ret or '<missing>'}, line={i + 1}")
j = i + 1
while j < len(text) and text[j].strip() == "":
j += 1
if j < len(text) and text[j].lstrip().startswith(('"""', "'''")):
print(f" docstring starts line {j + 1}: {text[j].strip()}")
else:
print(" docstring: <missing>")
PYRepository: NVIDIA/cuopt
Length of output: 11182
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=python/cuopt/cuopt/linear_programming/problem.py
printf '%s\n' '--- parent definitions ---'
git show HEAD^:"$file" | sed -n '35,65p;435,485p;1510,1570p'
printf '%s\n' '--- exact changed definitions ---'
git diff --unified=0 HEAD^ HEAD -- "$file" | rg -n -A18 -B4 \
'symbol|def __str__|def __repr__|^@@'
printf '%s\n' '--- annotations and Returns sections in public APIs ---'
rg -n -U \
'^[ ]{4}(def|async def) [A-Za-z_][A-Za-z0-9_]*\([^\\n]*\)( -> [^:]+)?:\\n|^[ ]{4}.*Returns?:' \
"$file" | head -n 160Repository: NVIDIA/cuopt
Length of output: 10805
Annotate the new CType.symbol property.
Add -> str and document the returned algebraic constraint symbol. The other display methods already existed.
📍 Affects 1 file
python/cuopt/cuopt/linear_programming/problem.py#L51-L55(this comment)python/cuopt/cuopt/linear_programming/problem.py#L343-L359python/cuopt/cuopt/linear_programming/problem.py#L1559-L1579
🤖 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 51 - 55,
Update the CType.symbol property in
python/cuopt/cuopt/linear_programming/problem.py at lines 51-55 to add a str
return annotation and document that it returns the algebraic constraint symbol;
apply the same annotation/documentation change to the sibling display methods at
lines 343-359 and 1559-1579 as requested.
Sources: Coding guidelines, Path instructions
There was a problem hiding this comment.
Pull request overview
This PR improves the usability of the Python Linear Programming (LP) modeling API in interactive contexts (notebooks/REPL) by adding algebraic __str__ and more descriptive __repr__ implementations for core modeling objects, and adds tests to lock in the expected formatting and truncation behavior.
Changes:
- Add algebraic string formatting (
__str__) and detailed object summaries (__repr__) forVariable,LinearExpression,QuadraticExpression,Constraint, andProblem. - Introduce shared expression formatting utilities (
_ExprBuilder,_format_linear) and output truncation via_MAX_DISPLAY_TERMS. - Add test coverage validating formatting details and truncation behavior for large expressions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| python/cuopt/cuopt/linear_programming/problem.py | Implements __str__/__repr__ across LP modeling classes and adds shared formatting utilities and truncation logic. |
| python/cuopt/cuopt/tests/linear_programming/test_python_API.py | Adds unit tests for the new string/representation behavior, including truncation for large expressions. |
| v1_str = str(var1) | ||
| v2_str = str(var2) | ||
| if v1_str == v2_str: | ||
| term_str = f"{v1_str}^2" | ||
| elif v1_str <= v2_str: | ||
| term_str = f"{v1_str} * {v2_str}" | ||
| else: | ||
| term_str = f"{v2_str} * {v1_str}" |
| index_to_var = {v.index: v for v in self.vars} | ||
| if self.is_quadratic: | ||
| for row, col, val in zip(self.rows, self.cols, self.vals): | ||
| builder.add_quadratic( | ||
| val, index_to_var[row], index_to_var[col] | ||
| ) | ||
| for idx, val in zip(self.linear_indices, self.linear_values): | ||
| builder.add_linear(val, index_to_var[idx]) | ||
| else: | ||
| for idx, coeff in self.vindex_coeff_dict.items(): | ||
| builder.add_linear(coeff, index_to_var[idx]) |
The Python LP modeling classes (Variable, LinearExpression, QuadraticExpression, Constraint, Problem) currently fall back to <cuopt.linear_programming.problem.X object at 0x...> when printed, which makes model construction hard to verify in notebooks and REPLs. This adds str (algebraic form, e.g.
2.0 * x + 3.0 * y <= 10.0) and repr (detailed summary with bounds, type, and variable/constraint counts and solve status) to all five classes. Purely additive; covered by est_str_and_repr in est_python_API.py.