Skip to content

Conversation

@saulshanabrook
Copy link
Member

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.

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 saulshanabrook force-pushed the redo-conversion-logic branch from 2ccaad7 to ea91781 Compare August 5, 2025 19:44
@saulshanabrook saulshanabrook requested a review from Copilot August 5, 2025 19:45
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 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_args test 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

@saulshanabrook saulshanabrook changed the title Add support for adding methods actually defined on classes Support methods like __array_function__ on expressions Aug 5, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented Aug 5, 2025

CodSpeed WallTime Performance Report

Merging #315 will not alter performance

Comparing redo-conversion-logic (90da0cf) with main (bf04275)

Summary

✅ 7 untouched benchmarks

@codspeed-hq
Copy link

codspeed-hq bot commented Aug 5, 2025

CodSpeed Instrumentation Performance Report

Merging #315 will not alter performance

Comparing redo-conversion-logic (90da0cf) with main (bf04275)

Summary

✅ 7 untouched benchmarks

@saulshanabrook saulshanabrook merged commit cbf9160 into main Aug 5, 2025
4 of 5 checks passed
@saulshanabrook saulshanabrook deleted the redo-conversion-logic branch August 5, 2025 20:05
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.

3 participants