Feature: Merge Expr and GenExpr
#1114
Open
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
Changed the type of _lhs and _rhs attributes in the ExprCons class from unspecified to 'object' to clarify their expected type and improve type safety.
Replaces the __init__ method with __cinit__ in the Variable class and updates the argument type to SCIP_VAR*.
Updated references from 'terms' to 'children' for Expr objects throughout Model methods to reflect changes in the Expr API. This ensures compatibility with the updated data structure and avoids errors when accessing expression terms.
Introduces _to_nodes methods for Expr, PolynomialExpr, and UnaryExpr to convert expressions into node lists for SCIP construction. Refactors Model's constraint creation to use the new node format, simplifying and clarifying the mapping from expression trees to SCIP nonlinear constraints.
Changed Expr from a Cython cdef class to a standard Python class for improved compatibility and maintainability. Removed cdef public dict children, as attribute is now managed in Python.
Zeroto521
force-pushed
the
issue/1074
branch
from
c13b346 to
8719b91
Compare
This reverts commit 28e6673.
Converted SumExpr, ProdExpr, and PowExpr from cdef classes to regular Python classes for improved compatibility and maintainability.
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.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Deleted the definition of the Expr class, which was not used in the code. This helps clean up the codebase and improves maintainability.
Changed ExprCons from a cdef class to a standard Python class and added type hints to the constructor parameters. This improves code readability and compatibility with Python tooling.
Type annotations were added to _normalize, __le__, and __ge__ methods in ExprCons. Error handling was improved by moving type checks earlier in __le__ and __ge__ to ensure arguments are Numbers before proceeding.
The __next__ method in Expr was unnecessary since iteration is handled by __iter__. This simplifies the class and avoids potential confusion.
Introduces the _first_child() method to Expr for cleaner child access. Updates PowExpr and UnaryExpr to use _first_child() in __repr__ and normalization logic, improving code readability and consistency.
Replaces use of '+=' for list concatenation with 'append' and 'extend' methods for clarity and consistency in node list construction across Term, Expr, PolynomialExpr, and UnaryExpr classes. This improves readability and ensures uniform handling of node lists.
Replaces integer initialization with ConstExpr for the result in PolynomialExpr's power operation to ensure correct expression type handling.
Renamed the static method _is_sum to _is_SumExpr for clarity and updated all references. Improved multiplication logic to handle sum expressions and self-multiplication cases more explicitly.
Changed Expr and ExprCons classes to cdef for performance and added public attributes. Updated Model methods to consistently use ExprCons type annotations and parameter names, improving type safety and clarity in constraint creation and addition.
Converted Term to a cdef class for Cython optimization, added explicit type annotations to methods, and removed runtime type checks in __mul__ and __eq__ for improved performance and clarity.
Refactored the _normalize method to directly filter out zero-valued children, removing the separate _remove_zero helper function for simplicity.
Operator overloads in the Variable class now delegate to MonomialExpr.from_var(self) instead of self.to_expr(). The to_expr() method was removed, simplifying the code and making the delegation explicit.
Renamed internal methods from _to_nodes to _to_node across Term, Expr, PolynomialExpr, and UnaryExpr classes for consistency. Updated logic to handle coefficient application and node construction more robustly. Adjusted Model class in scip.pxi to use the new method name.
Introduces a degree() method to the Variable class, returning the degree of the variable using MonomialExpr. This enhances the API for users needing variable degree information.
Replaced direct iteration over children.items() with iteration over self or self.children and explicit indexing in several methods of Term, Expr, PolynomialExpr, and UnaryExpr. This improves consistency and leverages custom __getitem__ implementations for coefficient access.
Refactors the Variable.__iadd__ method to delegate in-place addition to MonomialExpr.from_var(self).__iadd__(other), ensuring correct behavior and consistency with other arithmetic operations.
Moved the static method _is_SumExpr to the end of the Expr class and updated its signature to use Cython type annotation. Also refactored variable naming in the __add__children method for clarity and removed unnecessary blank lines.
Implements __iadd__ for ConstExpr and MonomialExpr to support in-place addition with other polynomial expressions. Also refactors _first_child to _fchild and updates references for consistency.
Changed _is_SumExpr from a static method to an instance method in the Expr class. Updated all usages to call the method on instances, improving code clarity and consistency.
Updated type annotations for several methods to improve type safety and clarity, especially for functions handling MatrixExpr. Refactored AbsExpr construction and improved handling of special cases in PowExpr. Enhanced consistency in operator overloads and function signatures.
Updated the degree method in the Variable class to return a float instead of an int, aligning the return type with MonomialExpr.from_var(self).degree().
The Term class constructor now raises a TypeError if any argument is not a Variable instance, ensuring type safety and preventing incorrect usage.
Changed the return type annotation of the __iter__ method in the Expr class to Iterator[Union[Term, Expr]], removing Variable from the union for improved type accuracy.
Simplifies Expr initialization by removing Variable from children keys and direct MonomialExpr conversion. Refactors unary function constructors (exp, log, sqrt, sin, cos) to accept numbers and variables directly, using a unified to_subclass method in UnaryExpr. This improves API usability and code clarity.
Unified and improved the _to_node method logic in Term, Expr, PolynomialExpr, and UnaryExpr classes. The refactor centralizes node construction in Expr, removes redundant overrides, and clarifies argument order for consistency. This change simplifies expression tree construction for SCIP integration.
Short-circuit addition in Expr and PolynomialExpr classes when adding a zero constant, returning the original expression. This improves efficiency by avoiding unnecessary object creation.
Introduces a 'copy' parameter to Expr.to_dict for controlling whether children are copied. Refactors PolynomialExpr.__iadd__ to use to_dict with copy=False for efficiency when merging children.
Updated __hash__ implementations in ProdExpr, PowExpr, and UnaryExpr to include the class type, ensuring correct hashing for different expression types with similar contents.
Improves the __iadd__ method for Expr to handle sum expressions more efficiently by modifying in place, and adds an __isub__ method for in-place subtraction. This enhances performance and consistency when using += and -= operators with Expr objects.
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.