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
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 <[email protected]>
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.
Updated the Expr constructor to clarify the allowed key types in the error message. Added tests to verify TypeError is raised for invalid keys and to check slot behavior in the Expr class.
Introduces a test to verify that Expr and its components enforce __slots__ by preventing dynamic attribute assignment, ensuring memory efficiency and attribute control.
Moved and grouped assertions in test_expr_op_expr for better logical flow. The sequence of operations and their corresponding type checks were reorganized, but the test coverage remains the same.
Introduces tests to verify the equality and type checking behavior of the Term class, ensuring correct comparison with other Term instances and raising TypeError for invalid comparisons.
Introduces test_getitem to verify correct indexing of Term objects, including valid access, type errors, and index errors.
Renamed test_init_error to test_Expr_init_error and test_slots to test_Expr_slots to improve test function naming consistency and clarity.
Added a new test 'test_Expr_getitem' to verify the __getitem__ behavior of Expr objects, including key lookups and error handling. Also added a .github/copilot-instructions.md file with architecture, development, and coding guidelines for contributors.
Introduces a new test to verify that applying abs() to an Expr returns an AbsExpr instance with the correct string representation and child expression.
Introduces unit tests to verify the behavior of the Expr._fchild() method with various expression compositions.
Adds tests to ensure that adding unsupported types (such as strings and lists) to Expr instances raises a TypeError.
Introduces tests to ensure multiplying an Expr by unsupported types (such as a string or list) raises a TypeError.
Introduces test_Expr_div to verify division behavior for Expr objects, including division by zero, scalar division, reciprocal, and division of expressions. Ensures correct exceptions and string representations are produced.
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.
Changed the parameter name from 'idx' to 'key' in the Term.__getitem__ method for consistency and clarity.
Implemented the __iter__ method for the Term class to allow iteration over its variables. Updated internal usage to leverage the new iterator.
Changed the _hash attribute in the Term class to be readonly and updated the __eq__ method to check for instance type before comparing hashes. This improves type safety and encapsulation.
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.