Skip to content
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

fix: register Numba in test #2879

Merged
merged 1 commit into from
Dec 8, 2023
Merged

fix: register Numba in test #2879

merged 1 commit into from
Dec 8, 2023

Conversation

agoose77
Copy link
Collaborator

@agoose77 agoose77 commented Dec 8, 2023

This PR recognises that Numba isn't being registered for a specific test, and fixes it!

@agoose77 agoose77 requested a review from ianna December 8, 2023 12:06
@agoose77 agoose77 force-pushed the agoose77/fix-numba-test-ufunc branch from 5686284 to d36a8b0 Compare December 8, 2023 16:21
@agoose77 agoose77 changed the title fix: register Numba for test, add ufunc signatures fix: register Numba in test Dec 8, 2023
@agoose77
Copy link
Collaborator Author

agoose77 commented Dec 8, 2023

@jpivarski I simplified this to only address a missing ak.numba.register_and_check().

Copy link

codecov bot commented Dec 8, 2023

Codecov Report

Merging #2879 (d36a8b0) into main (9400780) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Copy link
Collaborator

@ianna ianna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@agoose77 - I think we also need to check on the numba version greater or equal to 0.58 to avoid specifying the types for vectorise. Perhaps, adding a warning in case the version does not support it yet?

Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

About the minimum Numba version: it's true that @nb.vectorize without explicit arguments won't work before a certain Numba version, but that could be described as something missing from Numba, rather than something missing from Awkward.

In addition, @nb.vectorize is not a very popular function. This plot comes from static analysis of all the non-fork repos that import numba (or from numba import):

image

(If Numba is imported as another name, like nb, this includes nb.vectorize, and the nb.jit and nb.njit have been folded such that nb.jit with nopython=True counts as nb.njit only.)

So it doesn't seem necessary to me for Awkward to require a much newer Numba version just for vectorize. Since the Numba dependency is not managed by pip/conda (it's an optional dependency), dealing with mismatched version numbers is more annoying, as it's something the user must do manually.

@jpivarski
Copy link
Member

All of the tests pass. @ianna, if you agree, you can do the merge. If not, let me know your reasons and we'll talk!

Copy link
Collaborator

@ianna ianna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @jpivarski! Yes, I agree, let's merge it!

@ianna ianna merged commit f161bd1 into main Dec 8, 2023
39 of 40 checks passed
@ianna ianna deleted the agoose77/fix-numba-test-ufunc branch December 8, 2023 21:15
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