-
Notifications
You must be signed in to change notification settings - Fork 143
Cleanup numba dispatches #1648
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Cleanup numba dispatches #1648
Conversation
ec3b6be
to
7872d99
Compare
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (90.70%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1648 +/- ##
==========================================
+ Coverage 81.58% 81.63% +0.05%
==========================================
Files 240 242 +2
Lines 53593 53564 -29
Branches 9454 9451 -3
==========================================
+ Hits 43722 43727 +5
+ Misses 7395 7360 -35
- Partials 2476 2477 +1
🚀 New features to boost your workflow:
|
Thanks, I will start reviewing this PR this weekend |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few questions otherwise that looks good to me!
return lambda x, y: False | ||
|
||
|
||
enable_slice_boxing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is a function call missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that could mean we actually don't need (or are not testing) this functionality... Should show up in runtime slices
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a test, and actually fixed it a bit (last commit). This can all be removed after #541 is fixed, as we won't need to represent slices symbolically anymore
7872d99
to
3521a40
Compare
They are actually defined in tensor/math.py, but this is better than being in `basic.py`
Helpers before dispatchers
This isn't strictly needed but it's a more intuitive placement
3521a40
to
c594eb4
Compare
Spinoff some cleanup from #1637
There was a lot of cruft in
dispatch/basic.py
, not to be confused withdispatch/tensor_basic.py
, including a duplicated dispatch of Solve?This PR moves things to better files, removes unused functions, and makes other obsolete.
It removes the helper
to_scalar,
that made numba dispatchers more permissive than they need to be?This might have been originally here due to numba/numba#10266 but the work-around should happen at the relevant Elemwise op level, not in functions that accept 0d arrays. Otherwise we end up chasing this everywhere (and probably missing it in some places)
📚 Documentation preview 📚: https://pytensor--1648.org.readthedocs.build/en/1648/