Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Conversation

@Zeroto521
Copy link
Contributor

Close to #1074. This is a breaking change. We can release the 5.7.0 version first.

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.
Converted SumExpr, ProdExpr, and PowExpr from cdef classes to regular Python classes for improved compatibility and maintainability.
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 children replacing terms
  • Variable class no longer inherits from Expr
  • 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.

src/pyscipopt/expr.pxi Outdated Show resolved Hide resolved
Zeroto521 and others added 3 commits November 18, 2025 18:35
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.
Renamed 'test_expr.py' to 'test_Expr.py' and significantly expanded the test coverage for the Expr class and related operations. The new test file includes more granular and comprehensive tests for initialization, arithmetic operations, normalization, degree, node conversion, and equality, improving maintainability and reliability of the Expr implementation.
Eliminated the explicit isinstance check for 'other' in Expr._merge_dicts, allowing the method to assume 'other' is a dict. This simplifies the code and relies on duck typing for compatibility.
Renamed loop variables in the __imul__ method from 'k, v' to 'child, coef' for improved code clarity and consistency.
Return an empty list when the coefficient is zero in Expr._to_node, preventing unnecessary node construction for zero terms.
Corrected indentation in the Expr class when handling sum expressions to improve code readability and maintain consistency.
Refactors the __imul__ method in the Expr class to use a dictionary comprehension for updating child coefficients, improving readability and efficiency.
Replaces the use of Term with Variable when constructing expression nodes in expr.pxi and updates the corresponding type check in scip.pxi. This ensures that variable nodes are correctly identified and processed in the model's expression handling.
Renamed MonomialExpr.from_var to _from_var and updated all internal references to use the new private method. This change clarifies that the method is intended for internal use only.
Implemented __eq__ in ExprCons to raise NotImplementedError, clarifying that only '<=' or '>=' operations are supported. Also made a minor adjustment to the ValueError message for consistency.
Eliminated unnecessary checks for None on _lhs and _rhs in ExprCons methods, as these conditions are already handled elsewhere. Also added readonly declarations for _lhs and _rhs.
Simplified __le__ and __ge__ methods in ExprCons by removing redundant type checks and using direct float type annotations for the 'other' parameter.
Changed constructor argument type annotations from Python type hints to Cython-style type declarations for 'constant', 'coef', and 'expo' in ConstExpr, ProdExpr, and PowExpr classes. This improves compatibility with Cython and may enhance performance.
Replaces all uses of MonomialExpr with PolynomialExpr for variable expressions. Removes the MonomialExpr class and updates related methods and static constructors to use PolynomialExpr instead, simplifying the expression hierarchy.
Simplifies and centralizes the logic for checking zero expressions in the Expr class. The _is_zero method now handles empty and constant-zero cases, and redundant checks for falsy values are removed from __add__ and __mul__.
Changed ConstExpr(0) to ConstExpr(0.0) in the Expr class to ensure floating point comparison, improving type consistency in expression handling.
Changed the _to_subclass class method to explicitly type the 'cls' parameter as Type[PolynomialExpr] for improved type clarity.
Corrects the order of arguments passed to UnaryExpr._to_subclass in exp, log, sqrt, sin, and cos functions, ensuring the class type is provided as the first argument and the expression as the second. Updates type hints to reflect the return type as Union[UnaryExpr, MatrixExpr].
Simplified and unified the __iadd__ implementation for Expr, PolynomialExpr, and ConstExpr by moving logic to Expr and removing redundant overrides. Added _to_subclass helper for correct subclass handling during in-place addition.
Changed the 'expr', '_lhs', and '_rhs' attributes of the ExprCons class from public to readonly to prevent external modification and improve encapsulation.
Updated the Term class to make the 'vars' attribute readonly instead of public, and changed the _ExprKey class to make the 'expr' attribute readonly. This enhances encapsulation and prevents external modification of these attributes.
Replaces __le__, __ge__, and __eq__ with a unified __richcmp__ method in Expr and ExprCons classes for more robust and maintainable comparison logic. Updates class definitions to use 'cdef class' for consistency and adds cdef readonly attributes where appropriate.
Updated Term.__eq__ to compare hashes dynamically using hash(self) and hash(other) instead of relying on the _hash attribute. Also removed the unused static method Expr._to_subclass.
Adjusted test assertions in test_Term.py to expect (Variable, x) and (Variable, y) tuples instead of (Term, x) and (Term, y) in the output of Term._to_node(). This reflects a change in the internal representation of Term nodes.
Replaces the static method _to_subclass with an instance method _to_polynomial in the Expr class, updating internal usage accordingly. Also changes _children from public to readonly for better encapsulation.
Simplified the __hash__ implementations in Expr, ProdExpr, PowExpr, and UnaryExpr by removing type information from the hash tuple. Also removed the redundant __hash__ method from PolynomialExpr. Added tests for variable comparison operators to verify correct string representations.
Explicitly casts the result of self.__add__(-other) to Expr in comparison operator implementations to ensure correct type handling in ExprCons construction.
Moved the __slots__ declaration after the cdef readonly attribute in ProdExpr and PowExpr classes for consistency and clarity.
Updated quicksum and quickprod to use cython cpdef signatures with explicit Iterator[Expr] typing and cdef local variables for improved performance and type safety. Enhanced docstrings to clarify parameters and return values.
Refactored exp, log, sqrt, sin, and cos functions to use a unified decorator for matrix support and a Cython helper for expression construction. Improved setup.py by adding numpy include directory and enhancing cythonize options. These changes improve maintainability, consistency, and compatibility with numpy arrays.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.