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.
Zeroto521
force-pushed
the
issue/1074
branch
from
3e0359d to
da49cca
Compare
This reverts commit 659fb2b.
Introduced a shared _remove_zero() method in SumExpr to eliminate zero-valued children, and updated normalization methods in SumExpr and PolynomialExpr to use this helper for improved code reuse and clarity.
Refactored __add__ in Expr to return 'other' when 'self' has no children, ensuring correct addition semantics and simplifying logic.
Changed the expected degree of an empty Expr from 0 to float('inf') in test_degree to reflect updated behavior.
Improves multiplication handling in Expr and ProdExpr classes. Expr now multiplies by a constant using dictionary comprehension, and ProdExpr prevents duplicate children and simplifies multiplication logic for constants.
Merged SumExpr functionality into Expr, simplifying sum expression logic and normalization. Updated PolynomialExpr to inherit directly from Expr. Adjusted Model class to handle Expr instead of SumExpr for sum expressions. This refactor streamlines expression management and reduces class complexity.
Moved Cython and C imports below standard library imports for better organization. Reformatted the addMatrixVar method signature and the addCons loop for improved readability and consistency.
Implements the __iadd__ method for PolynomialExpr, allowing in-place addition of polynomial expressions by updating child coefficients. Falls back to superclass behavior for non-polynomial operands.
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. |
Replaces use of variable pointers for hashing and equality in the Term class with Python's built-in hash function on the sorted variable tuple. This simplifies the implementation and improves consistency. Also updates degree check in node conversion.
Introduced __slots__ to Expr, ProdExpr, and PowExpr classes to reduce memory usage and improve attribute access performance.
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.
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.
Introduces new test cases in test_PowExpr.py to verify the behavior of the PowExpr._to_node method with various inputs, including default and non-default exponents. This enhances test coverage for expression tree serialization.
Added an import for Variable from pyscipopt.scip to expr.pxi, likely to enable usage of the Variable type in this module.
Refactored the class name UnaryOperator to UnaryOperatorMixin in expr.pxi, scip.pxd, and scip.pxi for clarity and to better reflect its use as a mixin. Updated all relevant class inheritance accordingly.
Moved the hash logic directly to __hash__ and updated ptr() to return the hash. Also simplified __iter__ to directly call Expr._from_var(self).__iter__().
Added a check in ProdExpr to require at least two children and updated related tests to reflect this constraint. Removed or modified tests that previously allowed ProdExpr with fewer than two children.
Refactored the _to_node method in Expr and moved specialized implementations to ConstExpr, ProdExpr, PowExpr, and UnaryExpr. This change ensures correct coefficient propagation and node construction for each expression type. Updated related tests to match the new node structure and coefficient handling.
Replaces direct CONST dictionary access with a new inline function _c(expr) for extracting constant values from expressions. This improves code readability and maintainability by centralizing constant extraction logic.
Introduced a new test case to verify the behavior of negating a ProdExpr, ensuring correct type and string representation for both the negated and original expressions.
Refactored the _normalize method in ProdExpr to return ConstExpr(0.0) when the expression is empty or has zero coefficient, otherwise return self. Updated related tests to reflect the new normalization behavior and added a test for multiplication by zero.
Replaces direct instantiations of ConstExpr with the new _const helper function throughout expr.pxi. This change centralizes constant expression creation, improving consistency and maintainability.
Moved CONST, _c, _const, _wrap, _unwrap, _to_expr, _is_sum, _is_zero, and _fchild helper definitions to the end of the file for better organization. No functional changes were made.
Replaces static methods Expr._is_const and Expr._is_term with inline functions _is_const and _is_term. Updates all usages to call the new inline functions, improving code clarity and consistency.
Explicitly casts the result of cls.__new__(cls) to Expr in the copy method to ensure correct type handling and avoid potential type errors.
Replaces direct construction of unary expression types with a new _to_unary helper function for consistency and code reuse. Updates both operator overloads and numpy ufunc handling to use this helper.
Replaces direct calls to PolynomialExpr.create with the new _polynomial inline function for constructing PolynomialExpr instances. Removes the static create method from PolynomialExpr, consolidating instance creation logic and improving code clarity.
Removed the static create method from the Term class and replaced all usages with the new _term inline function. This change simplifies term creation and centralizes the logic for constructing Term instances.
The 'inline' keyword was removed from multiple Cython cdef functions in expr.pxi. This change may improve compatibility or resolve issues related to Cython compilation or function inlining.
Replaces direct access to the _children attribute of expression objects with the public items() method throughout the Model class. This improves encapsulation and future-proofs the code against changes in the internal structure of expression objects.
Replaces direct Expr instantiation with the new _expr helper function for constructing Expr objects from dictionaries. This change improves consistency and encapsulation in expression creation.
Replaces direct instantiation of PolynomialExpr with a more generic _expr factory function that accepts the target class as an argument. This change improves code reuse and consistency in expression construction.
Replaces direct PowExpr and ProdExpr instantiation with helper functions _pow and _prod for consistency and encapsulation. Updates test to provide required exponent argument to PowExpr.
Removed the default value for the 'expo' parameter in PowExpr's constructor, requiring callers to always specify the exponent. Updated related test to provide the exponent explicitly.
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.