Support methods like __array_function__ on expressions#315
Merged
Conversation
Adds support for methods like `__array_function__` which actually need to be added on the class as actual methods, not through overloading `__getattr__`. Custom methods can be registered by third party libraries. This PR also redoes the logic for upcasting when using binary operations. Instead of upcasting both values, it will only ever upcast one, choosing whichever one would be cheaper to upcast. This leads to more predictable behavior.
saulshanabrook
force-pushed
the
redo-conversion-logic
branch
from
2ccaad7 to
ea91781
Compare
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for methods that need to be defined directly on classes as actual methods (not through __getattr__ overloading), such as __array_function__, and redesigns the binary operation upcasting logic to upcast only one operand (choosing the cheaper conversion) instead of both.
- Introduces a new mechanism for registering methods directly on runtime types for third-party library compatibility
- Completely refactors binary operation logic to find the minimum cost conversion between operands
- Removes the previous
test_upcast_argstest and updates related snapshot files
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| python/egglog/runtime.py | Core implementation of new method registration system and binary operation logic |
| python/egglog/declarations.py | Adds type matching methods for binary operations and type variable handling |
| python/egglog/conversion.py | Updates conversion utilities and removes old min conversion logic |
| python/egglog/egraph.py | Updates ignored method list to use new numeric binary methods constant |
| python/egglog/init.py | Exports new define_expr_method function |
| python/tests/test_high_level.py | Removes obsolete upcast test |
| python/tests/snapshots/test_array_api/ | Updates snapshots to reflect new expression optimization |
| docs/reference/python-integration.md | Documents the new method registration feature |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
saulshanabrook
changed the title
__array_function__ on expressions
CodSpeed WallTime Performance ReportMerging #315 will not alter performanceComparing Summary
|
CodSpeed Instrumentation Performance ReportMerging #315 will not alter performanceComparing Summary
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Adds support for methods like
__array_function__which actually need to be added on the class as actual methods, not through overloading__getattr__. Custom methods can be registered by third party libraries.This PR also redoes the logic for upcasting when using binary operations. Instead of upcasting both values, it will only ever upcast one, choosing whichever one would be cheaper to upcast. This leads to more predictable behavior.