abs(abs(x)) should return abs(x) rather than abs(abs(x))
#1176
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
Introduces a copy method to GenExpr and its subclasses for duplicating expression objects, with optional deep copy of children and coefficients. Also adds __abs__ support to UnaryExpr, enabling correct handling of absolute value expressions.
Introduces a test to verify that applying abs() twice to a variable results in the same string representation as applying it once, ensuring correct simplification in expression handling.
Documented that abs now returns itself as UnaryExpr(Operator.fabs) in the CHANGELOG.
Changed the return type of __abs__ in GenExpr from Incomplete to GenExpr and added the __abs__ method to UnaryExpr. This improves type hinting and consistency in the class hierarchy.
Changed the declaration of 'cls' in GenExpr.copy from a Python variable to a Cython object to ensure proper type handling.
Replaces the use of type(self) with Py_TYPE(self) in the GenExpr.copy method for more efficient class retrieval in Cython. Also reorders cimport statements for clarity.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR ensures that applying abs twice to an expression yields the same expression as a single abs, avoiding nested abs(abs(x)) trees that can slow constraint handling (e.g. addCons).
Changes:
- Add a regression test that asserts
str(abs(abs(x))) == str(abs(x))for a model variable. - Introduce a
GenExpr.copy()helper and overrideUnaryExpr.__abs__to return the existing absolute-expression (via a copy) instead of wrapping it again, while preserving behavior for other unary operators. - Update type stubs (
scip.pyi) and the changelog to reflect the new__abs__behavior onGenExpr/UnaryExpr.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
tests/test_expr.py |
Adds a focused test verifying that abs(abs(x)) produces the same printed form as abs(x), guarding against regressions in expression simplification. |
src/pyscipopt/scip.pyi |
Adjusts type hints so GenExpr.__abs__ and UnaryExpr.__abs__ are declared to return GenExpr, matching the implementation. |
src/pyscipopt/expr.pxi |
Adds a generic GenExpr.copy() helper and special-cases UnaryExpr.__abs__ to avoid nested absolute expressions while keeping evaluation semantics unchanged. |
CHANGELOG.md |
Documents the new behavior for abs on UnaryExpr(Operator.fabs, ...) under the Unreleased “Changed” section. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Changed the type of 'cls' from 'object' to 'type' in the GenExpr.copy method to ensure correct class instantiation using Py_TYPE.
The __abs__ method in the UnaryExpr class now includes a return type annotation, improving type clarity and static analysis.
The @disjoint_base decorator was removed from the UnaryExpr class in the type stub. This may reflect a change in class hierarchy or decorator usage.
Corrects attribute assignments in the GenExpr.copy method by explicitly casting to the appropriate subclass (SumExpr, ProdExpr, PowExpr) before setting subclass-specific attributes. This ensures proper copying of attributes and avoids potential attribute errors.
Explicitly cast the result of Py_TYPE(self) to <type> in the GenExpr.copy method for clarity and type safety.
Member
|
Hmm I'm not 10000% certain that this should be done in PySCIPOpt, it feels like it's something SCIP should do on its own. Same with squaring integer variables, for example. What do you think, @Zeroto521 ? |
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.
Current
abs(abs(x))will returnabs(abs(x)). This will slowaddConsand increase the difficulty to solve model. An easy way to fix this is to returnabs(x).