Feature: Merge Expr and GenExpr
#1114
Draft
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.
Added a check for __hash__ attribute before comparing hashes in Expr division to prevent errors when 'other' is not hashable.
Zeroto521
force-pushed
the
issue/1074
branch
from
3e0359d to
da49cca
Compare
Replaces hasattr(other, '__hash__') with isinstance(other, Hashable) for checking hashability in Expr division logic. This improves type safety and clarity when comparing hashes.
Added stricter type validation for Expr children and improved MonomialExpr.from_var to handle non-Variable input. Updated FuncExpr to raise an error for invalid children. These changes enhance robustness and error reporting in expression construction.
Unified and streamlined the _to_nodes method signatures and implementations across Term, Expr, PolynomialExpr, and UnaryExpr classes. This change improves consistency, supports coefficient handling, and simplifies node construction for SCIP expression building.
Type hints were added to methods and functions in expr.pxi for improved code clarity and static analysis. This includes specifying argument and return types for class methods and utility functions related to expressions.
Replaces custom _is_number functions with isinstance checks against numbers.Number in expr.pxi, matrix.pxi, and scip.pxi. This improves type safety and code clarity by leveraging the standard library's Number abstract base class.
Updated UnaryExpr to accept Number types directly and convert them to ConstExpr instances. This improves type handling and consistency when constructing unary expressions.
Type hints were added to methods in Term, Expr, PolynomialExpr, ConstExpr, ProdExpr, PowExpr, UnaryExpr, and ExprCons classes to improve code clarity and static analysis. Minor refactoring was performed for consistency in variable naming and method signatures.
Replaces the _to_unary_expr helper with a static method UnaryExpr.from_expr for creating unary expressions. Updates all relevant methods to use the new static method for consistency and improved encapsulation.
Changed the expected exception from NotImplementedError to TypeError when powering an expression with sqrt(2) in test_expr_op_expr. This reflects the actual exception raised by the code.
Changed quicksum and quickprod to use ConstExpr for initialization and updated parameter names and types for clarity. This improves performance and code readability by avoiding unnecessary intermediate data structures.
Refines the __add__ methods in Expr and SumExpr to better handle addition with SumExpr instances, ensuring correct merging of terms and consistent behavior when combining expressions.
Moved and unified the degree() method implementations for Term, Expr, PolynomialExpr, and FuncExpr classes. The degree calculation now consistently uses the degree() method of child elements, improving maintainability and correctness.
Corrects the logic for building node lists in Term and Expr classes to ensure proper handling of coefficients and child nodes. This improves the robustness of SCIP expression construction.
Updates the Expr.degree() method to return infinity when there are no children, instead of defaulting to zero. This ensures correct behavior for empty expressions.
Adds type checking for the expr argument and ensures at least one of lhs or rhs is provided in ExprCons. Moves validation logic from _normalize to __init__ for earlier error detection.
Initializes Expr children with {CONST: 0.0} by default instead of an empty dict. This ensures that expressions always have a constant term, which may help avoid issues with missing constants in further computations.
Updates the test to expect degree 0 for an empty Expr instead of infinity, aligning with the intended behavior of the Expr class.
Changed assertions in test_equation to expect lhs to be 1 instead of 0.0, reflecting updated behavior or requirements for equation evaluation.
Deleted the test_degree function from test_expr.py as it is no longer needed or used in the test suite.
Refines the __mul__ method in Expr to handle multiplication with zero and constant expressions more robustly. Also updates ConstExpr constructor to use float for default constant value.
Changed expected values in test_inequality to reflect updated behavior of GenExprs, asserting 1 instead of 0.0 for _lhs and _rhs.
Update __imul__ in ProdExpr to only modify coef for non-zero, non-one constants, removing special handling for zero. This simplifies the logic and avoids replacing the expression with a constant zero.
Replaces the UFUNC_DISPATCH dictionary with explicit inline handling of numpy ufuncs in the Expr class. This change improves clarity and removes the need for a separate dispatch dictionary.
This commit adds tests to verify that Expr objects support numpy array operations such as addition, subtraction, multiplication, division, power, negation, and comparison (equal, less_equal, greater_equal). These tests ensure correct behavior when Expr objects are used with numpy functions and arrays.
Replaces usage of the operator module with direct arithmetic expressions for numpy ufuncs in the Expr class. This simplifies the code and removes unnecessary indirection.
Replaced 'type(expr) is' with 'isinstance(expr, ...)' in test_abs and test_neg for better practice and compatibility with inheritance.
can't create ProdExpr like `ProdExpr(Term(x), Term(x))`
Introduced the test_cmp function to verify equality comparisons between ProdExpr expressions and between an expression and a constant. This enhances test coverage for expression comparison logic.
Replaced type(obj) is ... with isinstance(obj, ...) in test_Expr.py and test_PolynomialExpr.py for more robust type checking in test assertions.
Replaces direct calls to __hash__() on frozenset and tuples with the built-in hash() function for improved clarity and consistency in Expr, ProdExpr, PowExpr, and UnaryExpr classes.
Implemented __truediv__ for ProdExpr to support division by constants, variables, and expressions. Enhanced the _normalize method in both ProdExpr and PowExpr to handle cases with single children and to return simplified expressions when possible.
Introduces test_pow to verify correct behavior of the power (**) operator for PolynomialExpr and ConstExpr, including type and value checks, as well as error handling for invalid exponent types.
Introduces tests for division operations and normalization behavior in ProdExpr, including checks for division by constants, variables, and self, as well as normalization to ConstExpr, PolynomialExpr, and SinExpr. Enhances test coverage for expression handling in the symbolic math module.
The 'operator' module import was removed from expr.pxi as it was not being used in the file.
Replaces direct zero/one constant checks with calls to the _normalize() method in ProdExpr and PowExpr arithmetic operations. This centralizes normalization logic and improves code maintainability.
Introduces a static cdef method Term.create to standardize Term instance creation from variables. Replaces previous ad-hoc and _from_var methods with the new create method for consistency and code reuse.
Replaces the static _copy method with an instance copy method for Expr and introduces a static create method for PolynomialExpr. Updates all usages to use the new methods, improving clarity and maintainability of expression copying and instantiation.
Introduces a new test function to verify the normalization behavior of PowExpr for exponents 2.0, 1.0, and 0.0.
Eliminated __slots__ from Term, _ExprKey, Expr, ProdExpr, and PowExpr classes in expr.pxi. This may improve compatibility or flexibility, possibly to allow dynamic attribute assignment or inheritance.
Overrides the __neg__ method for Expr, FuncExpr, and ProdExpr to provide more efficient or correct negation behavior. Expr now negates its children, FuncExpr multiplies by -1, and ProdExpr negates its coefficient.
Refactored the degree methods in Term and FuncExpr classes to use def instead of cpdef, and added explicit return type annotations. This improves clarity and aligns with Python method conventions.
Replaces the Expr._fchild method with a standalone cdef inline _fchild function for accessing the first child in expression nodes. Updates all usages to use the new function, improving code clarity and consistency.
Moved _is_sum and _is_zero from static methods of Expr to module-level inline functions for improved readability and performance. Updated all internal references to use the new functions.
Expanded the test_is_equal function to include additional assertions for PowExpr, ensuring correct behavior and comparison with other expression types. This improves test coverage for expression key equality and inequality cases.
Renamed variable for clarity and improved type checks by replacing issubclass/type with isinstance. Updated handling of ProdExpr and PowExpr cases for more robust expression node construction.
Introduces unit tests for the _to_node method of the ProdExpr class, covering cases with empty and populated expressions, as well as multiplication with constants. This improves test coverage and helps ensure correct behavior of expression tree serialization.
Replaces the static method Expr._from_other with a new inline function _to_expr for converting numbers, variables, and expressions to Expr objects. Updates all relevant operator overloads and internal methods to use _to_expr, improving code clarity and maintainability.
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.