-
Notifications
You must be signed in to change notification settings - Fork 150
Numba full backend support and required dependency #811
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?
Conversation
2eb7fe7 to
1f093cd
Compare
|
Locally I ran all the tests in Numba caching is baaaaaaaaaaaaaaaaaaaaaaaaaaaaaaad |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
5c4491d to
a1e3775
Compare
This comment was marked as outdated.
This comment was marked as outdated.
6f6e8bb to
53d9a26
Compare
53d9a26 to
9076c99
Compare
9076c99 to
0af7cb7
Compare
|
After caching the test file now runs in 50s after caching vs 6s before the PR, so only 8x slower now :( |
0af7cb7 to
17ef6a4
Compare
bd3da41 to
5f17484
Compare
463cba4 to
3367ea8
Compare
e686b48 to
0b8ed71
Compare
Numba rewrites these to integers for specialized implementation
Mark overly specific tests as xfail
0b8ed71 to
bfd59a0
Compare
|
@maresb / @lucianopaz I requested your review basically for the last 2 commits, where we make numba default dependency and run it on the CI The prior commits is just tweaking tests so they pass with both backends. I've gone over them with @jessegrabowski and asked him to review those. Feel free to look if you want but nothing terribly interesting or critical there |
|
@maresb I force-pushed above your commit, sorry didn't see you had come to the rescue already. should we go with |
bfd59a0 to
4789ae2
Compare
4789ae2 to
78cde62
Compare
lucianopaz
left a comment
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've looked at the last two commits and they seem OK to me. My only question is why you decided to use python 3.12 or 3.13 on some extra included tests? I also can't see where you test float32.
That was already the case, I guess it was done to have minimal sanity check the library can run in the intermediate versions, since it was already special cased anyway. Re: float32 The parametrization is gone to keep CI from having too many jobs. Float32 tests are also an endless source of wasted runtime, as ppl forget to set different tolerance for comparison with numpy which always runs in float64. We can reassess this but I would like to try out and see how often does it bite us back. |
|
Also all tests passed in numba in float32 (and even FAST_COMPILE - I hacked the linker temporarily), so the checking was much more comprehensive. After passing, I then removed that commit and restricted the CI to its current shape |

What is broken / not supported by Numba:
The plan now
Immediate action
I'll be cherry picking the countless fixes into their own PRs. I need help with review as both me and @jessegrabowski are pretty much at capacityDone