Feature: Merge Expr and GenExpr
#1114
Draft
+3,094
−1,274
Conversation
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
Zeroto521
force-pushed
the
issue/1074
branch
from
c13b346 to
8719b91
Compare
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR merges the Expr and GenExpr classes into a unified expression system. The refactoring replaces the previous dual-system (polynomial Expr and general GenExpr) with a single hierarchy based on Expr, using specialized subclasses like PolynomialExpr, SumExpr, ProdExpr, and various function expressions (ExpExpr, LogExpr, etc.).
Key changes:
- Unified expression representation with
childrenreplacingterms Variableclass no longer inherits fromExpr- Simplified expression tree structure with improved type system
- Refactored constraint creation methods to use the new expression system
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/pyscipopt/scip.pyi | Updated Variable class to remove inheritance from Expr |
| src/pyscipopt/scip.pxi | Modified Variable class structure and added operator overloading methods to delegate to expression system; refactored constraint creation methods to use children instead of terms |
| src/pyscipopt/scip.pxd | Updated Variable class declaration to remove Expr inheritance |
| src/pyscipopt/propagator.pxi | Simplified variable creation by removing unnecessary temporary variable |
| src/pyscipopt/expr.pxi | Complete rewrite of expression system replacing Expr/GenExpr duality with unified class hierarchy |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Zeroto521
force-pushed
the
issue/1074
branch
from
3e0359d to
da49cca
Compare
Contributor
Author
|
Finish the base feature and function. And something needs to be done:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/pyscipopt/expr.pxi
Outdated
| raise TypeError("expr must be an Expr instance") | ||
| if lhs is None and rhs is None: | ||
| raise ValueError( | ||
| "Ranged ExprCons (with both lhs and rhs) doesn't supported" |
There was a problem hiding this comment.
The error message is grammatically incorrect. It should be "Ranged ExprCons (with both lhs and rhs) isn't supported" or "Ranged ExprCons (with both lhs and rhs) is not supported" instead of "doesn't supported".
Suggested change
| "Ranged ExprCons (with both lhs and rhs) doesn't supported" | |
| "Ranged ExprCons (with both lhs and rhs) is not supported" |
|
|
||
|
|
||
| class ExprCons: | ||
| """Constraints with a polynomial expressions and lower/upper bounds.""" |
There was a problem hiding this comment.
The docstring says "Constraints with a polynomial expressions" but should say "Constraints with polynomial expressions" (without "a" before "polynomial"). Also note that ExprCons can now handle non-polynomial expressions too (like exp, log, etc.), so the docstring is inaccurate and should be updated to reflect that it handles general expressions.
Suggested change
| """Constraints with a polynomial expressions and lower/upper bounds.""" | |
| """Constraints with general expressions and lower/upper bounds.""" |
Member
|
Thanks for your work @Zeroto521 !! This is something that should be very very well tested before merging. We're also in the middle of a SCIP meeting, so I can bring this up with the guys and we can quickly take a high level look. |
Introduces a test to verify that raising an expression to the power of 0 returns an expression equivalent to 1. Ensures correct handling of the power operation edge case.
Introduces a new test to verify the behavior of the Expr class when used as the exponent in the power operation (rpow). The test checks correct string output, and ensures appropriate exceptions are raised for invalid base types and negative bases.
Improves multiplication logic in Expr by handling cases where either operand has no children, returning a constant zero. Updates __slots__ in ProdExpr and PowExpr to only include relevant attributes, removing 'children' from both classes.
Replaces equality check with identity check in test_getitem to ensure the variable returned by Term is the same object as the original variable.
Renamed test_Expr_mul_unsupported_type to test_Expr_mul and added additional assertions to cover valid multiplication cases, including multiplication by zero, by an empty Expr, and by itself. This improves test coverage for the Expr multiplication operator.
Updated test cases in test_term.py to create explicit Model instances before adding variables. Added assertion for string representation in test_mul for improved test coverage.
Introduced a pytest fixture to create and share a Model, variable, and Term instance across tests. This reduces code duplication and improves test maintainability.
Replaces dictionary comprehension with dict.fromkeys for initializing the parent class in ProdExpr. This change simplifies the code and maintains the same functionality.
Simplifies the type conversion logic in UnaryExpr.to_subclass by removing the explicit conversion of Number to ConstExpr and adjusting the order of type checks.
Introduces a __bool__ method to the Expr class to allow direct truthiness checks. Refactors existing checks for self.children to use the new __bool__ method, improving code readability and consistency.
Refines addition and multiplication logic in Expr, PolynomialExpr, and ProdExpr to better handle cases involving constants 0 and 1. This improves efficiency and correctness by avoiding unnecessary object creation and ensuring proper simplification of expressions.
Replaces usage of the 'terms' variable with 'children' to improve clarity and consistency when accessing constraint expression children in Model methods. No functional changes were made.
Refactored the _to_node method in ProdExpr to return an empty list immediately if coef is zero, removing redundant code and improving readability.
Implemented exp, log, sqrt, sin, and cos methods for the ConstExpr class using Python's math module. This allows direct computation of these functions on constant expressions.
Replaces direct multiplication by 2.0 with _const(2.0) for consistency and corrects logic in __sub__ and __rsub__ methods to handle expression equality properly.
Eliminated unnecessary isinstance checks for the exponent in __pow__ and __rpow__ methods of Expr, PolynomialExpr, and ConstExpr. Type conversion and validation are already handled by _to_expr and subsequent logic.
Removed the __init__ method from UnaryExpr and simplified the _ensure_unary function to only accept Variable or Expr types. Updated function signatures and docstrings for exp, log, sqrt, sin, and cos to remove np.ndarray from return types, reflecting that only MatrixExpr or the respective Expr types are returned. Improved type safety and code clarity.
Replaces the UnaryOperatorMixin with a new ExprLike base class to unify the interface for expressions and variables. Refactors Expr, Variable, and related classes to use this new structure, moving operator overloads and common logic to ExprLike. Direct instantiation of Expr and Variable is now disallowed, and Variable now maintains an internal PolynomialExpr view for expression operations. Updates type checks and internal helpers to support the new design, improving maintainability and extensibility.
Replaces _is_const checks with explicit type checks for ConstExpr throughout expression operations for clarity and correctness. Introduces a _reset_hash helper to standardize hash invalidation, replacing direct assignments to _hash. Simplifies and corrects logic in several arithmetic and comparison methods, and updates __repr__ for UnaryExpr to improve output consistency.
Changed setObjective and chgReoptObjective to accept ExprLike instead of Expr, updated parameter types and default values for clarity, and removed an unnecessary assertion in setObjective. These changes improve type flexibility and documentation accuracy.
Simplifies the __mul__ method in PolynomialExpr by removing redundant checks and streamlining the multiplication logic. Also updates casting from Cython-style to Python-style in several places for consistency.
Refactored the matrix class hierarchy by renaming MatrixBase to MatrixExprLike in both matrix.pxi and scip.pxi. Updated all relevant class inheritances to improve code clarity and consistency.
Moved the conversion of 'other' to an Expr earlier in the __mul__ method to improve code clarity and ensure type checks use the converted value.
Updated the __hash__ and ptr methods in the Variable class to return size_t instead of Py_hash_t for consistency and type correctness.
Removed __array_priority__ from ExprLike and set MatrixExprLike's __array_priority__ to 100. This change ensures consistent behavior when interacting with NumPy ufuncs and resolves potential priority conflicts.
Replaces repeated (Number, Variable, Expr) type checks with centralized EXPR_OP_TYPES and NUMBER_TYPES tuples. This improves maintainability and consistency in type checking throughout the expression classes.
Type hints were removed from the __hash__ and ptr methods in the Variable class to improve compatibility and simplify the code.
Refactored Expr and related classes to use the public attribute 'children' instead of the internal '_children'. Updated all references and property definitions accordingly for consistency and clarity.
Updated references from cons.expr._children to cons.expr.children in the Model class to use the correct attribute for accessing constraint expression children. This change improves code clarity and ensures compatibility with the current expression API.
Improves detection and conversion of single-term polynomials by introducing _is_single_poly and _to_poly helpers. Removes redundant __add__ override in PolynomialExpr and streamlines sum logic in Expr to ensure correct type conversion for single polynomials. Updates __repr__ in UnaryExpr for consistency with new helpers.
Changed the first parameter of _expr_cmp from 'self' to 'expr' to improve clarity and consistency, updating all internal references accordingly.
Moved the _to_dict function from the Expr class to a standalone cdef function for improved clarity and reusability. Updated all internal calls to use the new function signature and removed the method declaration from the class definition.
Refines handling of exponentiation for Expr and PolynomialExpr classes, including special cases for exponents 0 and 1, and optimizes polynomial multiplication by using a dictionary for intermediate results. These changes improve correctness and efficiency in mathematical operations.
Introduces a new _normalize function to handle normalization and coefficient scaling of expression children. Replaces multiple dictionary comprehensions with calls to _normalize for improved code reuse and clarity.
Refactored the _to_dict function to use C-level dict access for improved efficiency and to handle zero coefficients correctly. This change avoids unnecessary copying and ensures more accurate updates when combining expression children.
Refactors the __mul__ method in PolynomialExpr to use PyDict_Next for iterating over dictionary items, improving performance and reducing Python overhead. This change also skips zero coefficients and accumulates products directly in the result dictionary.
Replaces the use of next(iter(expr.children)) with PyDict_Next for more direct and efficient access to the first child key in Expr. Raises StopIteration if Expr has no children.
Refactored the __mul__ method in the Term class to efficiently merge variable tuples while maintaining order based on hash values. Introduced a new _extend helper function for extending lists from tuple slices, improving performance and code clarity.
Replaces direct tuple item comparison with explicit Variable extraction and identity check in the Term class equality method. This ensures correct behavior when comparing Term objects.
Removed unnecessary checks for coefficient equal to 1 in multiplication logic and updated normalization to directly copy children when coefficient is 1. This streamlines expression handling and avoids redundant copying.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Close to #1074. This is a breaking change. We can release the 5.7.0 version first.