Uh oh!
There was an error while loading. Please reload this page.
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 5 commits into
Conversation
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:
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)
Included review availability: Your plan includes up to 12 reviews per rolling hour; 11 remain after this review. 📝 WalkthroughWalkthroughThis PR adds readable ChangesHuman-readable display support for LP API objects
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk:⚪ Minimal · up to This localized additive change improves string and representation output for LP API classes without introducing an actionable merge-blocking risk; it is merge-ready after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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
Uh oh!
There was an error while loading. Please reload this page.
chris-maes
commented
Jun 9, 2026
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)>. |
jackthepunished
commented
Jun 15, 2026
@chris-maes can you /review |
ramakrishnap-nv
commented
Jun 22, 2026
@chris-maes May I get your review on this PR |
mlubin
commented
Jun 26, 2026
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
There was a problem hiding this comment.
Requesting the changes I mentioned above.
aca0220 to
cd6c5caCompareThere 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
Uh oh!
There was an error while loading. Please reload this page.
| 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. |
mlubin
commented
Jul 29, 2026
@jackthepunished do you plan to follow up on this PR? |
jackthepunished
commented
Jul 30, 2026
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. |
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/bashset -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/bashset -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/bashset -euo pipefail
python3 - <<'PY'import astfrom pathlib import Pathpath = 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/bashset -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 Pathimport reimport systext = Path(sys.argv[1]).read_text().splitlines()targets = {"symbol", "__str__", "__repr__"}class_name = Nonefor 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/bashset -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}" |
There was a problem hiding this comment.
Agreed, check on equality on the variables, no equality on the variable strings.
There was a problem hiding this comment.
Agreed, fixed. One wrinkle worth flagging: == isn't usable here because Variable.__eq__ is overloaded to build a Constraint (which also leaves Variable unhashable), so the check compares by identity first and then by index when both variables are attached to a problem. The string comparison stays only for the cosmetic ordering of cross terms. Added a test with two distinct variables sharing the name x, asserting x * x rather than x^2.
| 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]) |
There was a problem hiding this comment.
Please make sure this use case is tested
🔔 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. |
1 similar comment
🔔 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. |
| if self.VariableName: | ||
| return self.VariableName | ||
| if self.index >= 0: | ||
| return f"C{self.index}" |
There was a problem hiding this comment.
Is this meant to be f"V{self.index}"?
There was a problem hiding this comment.
C is deliberate rather than a typo: Problem._to_data_model already names unnamed variables C{index} on export (and unnamed rows R{index}), following the MPS column/row convention, so str(var) lines up with what you see in an exported MPS file or in a solution. Happy to switch to V{index} if you'd rather printing be independent of the export naming, say which you prefer and I'll change it.
| if self.index >= 0: | ||
| return f"C{self.index}" | ||
| # Not yet added to a problem: no name and no index to show. | ||
| return f"V{id(self)}" |
There was a problem hiding this comment.
id(self) is a strange fallback, maybe just fall back to __repr__?
There was a problem hiding this comment.
Agreed it's ugly. Falling back to __repr__ unfortunately recurses, since __repr__ interpolates {self}: __str__ -> __repr__ -> __str__. I've changed it to read like a default repr instead, <Variable at 0x{id(self):x}>, which keeps it unique per object so several detached variables in one expression don't collapse to the same token. Note this branch is only reachable by constructing Variable() directly rather than going through Problem.addVariable.
| def __repr__(self): | ||
| vtype = self.VariableType | ||
| if isinstance(vtype, (bytes, bytearray)): | ||
| # The MPS data model yields variable types as byte codes. |
There was a problem hiding this comment.
I don't know if this behavior of VariableType is intended, could you open an issue about this behavior?
There was a problem hiding this comment.
Will do. It does look unintended: _to_data_model builds var_type with dtype="S1", and _from_data_model passes dm.get_variable_types()[i] straight into addVariable(vtype=...), so a problem read from an MPS file ends up with VariableType == b"I" rather than VType.INTEGER. There's already a workaround for it in the IsMIP property (if var.VariableType in ("I", "S", b"I", b"S")). I'll open an issue with that trace and link it from here. The decode() has to stay in the meantime, since VType(b"I") raises ValueError.
There was a problem hiding this comment.
Opened #1736. One correction to what I wrote above: readMPS yields plain "I" strings (the parser builds var_types with dtype='str'), not bytes; bytes only arrive when a caller passes them, and addVariable/setVariableType accept them unchecked. So the real problem is that VariableType isn't normalized to VType — three representations coexist and IsMIP / solver.is_mip each cope ad hoc. Details and repro in the issue.
| 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}" |
There was a problem hiding this comment.
Agreed, check on equality on the variables, no equality on the variable strings.
| @@ -1341,7 +1493,14 @@ def __init__(self, expr, sense, rhs, name=""): | |||
| self.rhs_value = rhs_value | |||
| self.RHS = rhs_value | |||
| self.vindex_coeff_dict = {} | |||
| self.vars = expr.vars | |||
| # All participating variables (linear and quadratic terms), so | |||
There was a problem hiding this comment.
I'm not sure I follow, but this seems like a pre-existing separate issue. If so it should be reviewed and fixed in a separate PR.
There was a problem hiding this comment.
You're right that it's pre-existing: on main the quadratic branch does self.vars = expr.vars, and a QuadraticExpression's vars holds only the linear variables — so anything appearing solely in a quadratic term never lands in Constraint.vars. Nothing reads it for quadratic rows today, which is why printing is the first thing to trip over it.
Agreed on splitting it out: I'll open it as its own PR against main with tests and stack this one on top, so the data-model change gets reviewed on its own and this PR shrinks to just the printing once that lands. That also settles your earlier memory concern properly, since #1400 then carries no data-model changes at all. (For the record, the dedup only held pointers to already-live Variables rather than a second copy of the coefficients; the id() keying is forced by Variable being unhashable, same reason as the equality thread above.)
| 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]) |
There was a problem hiding this comment.
Please make sure this use case is tested
Quadratic rows stored only the variables of their linear terms, and updateConstraint did not record variables it introduced, so mapping a row's column indices back to Variables (compute_slack) could KeyError. Signed-off-by: jackthepunished <kosapinarbahadir@gmail.com>
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>
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>- 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.
- Compare variables by identity/index in quadratic terms, not by name - Fall back to __repr__ for a Variable without name or index - Test updateConstraint-introduced variables and duplicate names - Move the after-solve Problem.__str__ check into its own test - Assert exact truncation strings Signed-off-by: jackthepunished <kosapinarbahadir@gmail.com>
26fc820 to
55b66b7CompareNote GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
jackthepunished
commented
Aug 18, 2026
@mlubin I've pushed the follow-ups. The Also wanted to properly close the loop on your original point about large expressions, since I never answered it head-on. Right now an expression prints its first 10 non-zero terms and then Two things I'd rather ask than guess: is head-only with a cap of 10 fine, or would you prefer something like JuMP does (head and tail, omitted count in the middle, bigger cap)? And for notebooks, is plain text the same as the REPL good enough for this PR, with |
Stacked on #1737; its commit appears here until it merges.
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, variable/constraint counts and solve status) to all five classes. Expressions past 10 terms end in... (N more terms). Purely additive; covered bytest_str_and_repr,test_problem_str_after_solveandtest_str_truncation_large_expression.Variable.__repr__decodes byte-valuedVariableTypeuntil #1736 is addressed.