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 2 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>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds human-readable ChangesHuman-readable display support for LP API objects
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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>
jackthepunished
force-pushed
the
feature/str-repr-for-lp-api
branch
from
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
| # === QuadraticExpression also truncates === | ||
| qexpr = xs[0] * xs[0] | ||
| for i in range(1, n): | ||
| qexpr = qexpr + xs[i] * xs[i] | ||
| qs = str(qexpr) | ||
| assert qs.count("^2") <= cap | ||
| assert qs.endswith(f"... ({n - cap} more terms)") | ||
| assert repr(qexpr) == f"<cuopt.QuadraticExpression: {qs}>" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Tighten the quadratic truncation assertion.
Line 996 only checks <= cap, so a regression that renders fewer than _MAX_DISPLAY_TERMS quadratic terms would still pass. This test should pin the exact truncation boundary the same way the linear branch does. As per path instructions, "Focus review signal on correctness/API behavior for the new __str__/__repr__ outputs and their truncation/tests."
Suggested test tightening
- assert qs.count("^2") <= cap
+ assert qs.count("^2") == cap
+ assert qs.startswith("x0^2 + ")📝 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.
| # === QuadraticExpression also truncates === | |
| qexpr = xs[0] * xs[0] | |
| for i in range(1, n): | |
| qexpr = qexpr + xs[i] * xs[i] | |
| qs = str(qexpr) | |
| assert qs.count("^2") <= cap | |
| assert qs.endswith(f"... ({n - cap} more terms)") | |
| assert repr(qexpr) == f"<cuopt.QuadraticExpression: {qs}>" | |
| # === QuadraticExpression also truncates === | |
| qexpr = xs[0] * xs[0] | |
| for i in range(1, n): | |
| qexpr = qexpr + xs[i] * xs[i] | |
| qs = str(qexpr) | |
| assert qs.count("^2") == cap | |
| assert qs.startswith("x0^2 + ") | |
| assert qs.endswith(f"... ({n - cap} more terms)") | |
| assert repr(qexpr) == f"<cuopt.QuadraticExpression: {qs}>" |
🤖 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
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.
Source: Path instructions
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.
4 participants
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.