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

feat: add CUDA kernels that calculate length/sum #2992

Merged
merged 21 commits into from
Feb 7, 2024

Conversation

ManasviGoyal
Copy link
Collaborator

@ManasviGoyal ManasviGoyal commented Jan 30, 2024

  • New kernels added -

    1. awkward_Index_nones_as_index
    2. awkward_ByteMaskedArray_numnull
    3. awkward_IndexedArray_numnull
    4. awkward_IndexedArray_numnull_parents
    5. awkward_ListArray_getitem_jagged_carrylen
    6. awkward_ListArray_getitem_next_range_counts
    7. awkward_ListArray_rpad_and_clip_length_axis1
    8. awkward_ListOffsetArray_reduce_nonlocal_nextstarts_64
    9. awkward_RegularArray_getitem_next_array_regularize
    10. awkward_RegularArray_reduce_local_nextparents
    11. awkward_RegularArray_reduce_nonlocal_preparenext
    12. awkward_sorting_ranges_length
  • Makes awkward_ListArray_min_range more efficient by using cupy.min CUDA kernels that are implemented but not optimal #2987

  • Fixes the implemetation of many existing kernels

  • Adds unit-tests

@ManasviGoyal ManasviGoyal added the gpu Concerns the GPU implementation (backend = "cuda') label Jan 30, 2024
Copy link

codecov bot commented Jan 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b749e49) 81.90% compared to head (9a73593) 81.89%.
Report is 3 commits behind head on main.

❗ Current head 9a73593 differs from pull request most recent head 7fae36f. Consider uploading reports for the commit 7fae36f to get more accurate results

Additional details and impacted files
Files Coverage Δ
src/awkward/_connect/cuda/__init__.py 0.00% <ø> (ø)

... and 7 files with indirect coverage changes

@ManasviGoyal ManasviGoyal changed the title feat: add cumulative sum CUDA kernels feat: add CUDA kernels that calculate length Jan 31, 2024
@ManasviGoyal ManasviGoyal changed the title feat: add CUDA kernels that calculate length feat: add CUDA kernels that calculate length/sum Jan 31, 2024
@ManasviGoyal ManasviGoyal force-pushed the ManasviGoyal/add-cumulative-sum-kernels branch from feca8b2 to fdcf96e Compare January 31, 2024 16:12
@ManasviGoyal
Copy link
Collaborator Author

ManasviGoyal commented Feb 2, 2024

@jpivarski I am done with this PR but I am not quite sure why the integration test is failing. All the tests pass locally.

Edit: Oh, I just noticed #2998 which refers to this issue. I need to wait for that PR to merge.

@ManasviGoyal ManasviGoyal changed the title feat: add CUDA kernels that calculate length/sum feat: add CUDA kernels that calculate length/cumsum Feb 2, 2024
@ManasviGoyal ManasviGoyal changed the title feat: add CUDA kernels that calculate length/cumsum feat: add CUDA kernels that calculate length/sum Feb 2, 2024
@ManasviGoyal ManasviGoyal marked this pull request as ready for review February 2, 2024 13:46
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.

These look great! I also tested everything on my GPU; there were no errors. We should not merge until after 2.6.0, though.

@ManasviGoyal
Copy link
Collaborator Author

These look great! I also tested everything on my GPU; there were no errors. We should not merge until after 2.6.0, though.

@jpivarski Can this be merged now?

@jpivarski
Copy link
Member

Thanks for formatting it. In the changes since I last viewed, it looks like a lot more kernels were touched. Was it the case that only a few of them were previously formatted in black-style, and now you did all of them?

I think this PR will change awkward-cpp, and I want to release another awkward version, 2.6.1 with #3006, so please hold off on merging this for a little while longer. Thanks!

@ManasviGoyal
Copy link
Collaborator Author

ManasviGoyal commented Feb 5, 2024

Was it the case that only a few of them were previously formatted in black-style, and now you did all of them?

Yes, there were very few that were formatted earlier.

I think this PR will change awkward-cpp, and I want to release another awkward version, 2.6.1 with #3006, so please hold off on merging this for a little while longer. Thanks!

Sure. Thank you.

@jpivarski
Copy link
Member

Awkward 2.6.1 is out, so this can be merged whenever you want. It is understood that the next version will require a new awkward-cpp. (Unfortunately, that might slow down testing of #3007. Oh well.)

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b749e49) 81.90% compared to head (9a73593) 81.89%.
Report is 10 commits behind head on main.

❗ Current head 9a73593 differs from pull request most recent head 71e305e. Consider uploading reports for the commit 71e305e to get more accurate results

Additional details and impacted files
Files Coverage Δ
src/awkward/_connect/cuda/__init__.py 0.00% <ø> (ø)

... and 7 files with indirect coverage changes

@ManasviGoyal
Copy link
Collaborator Author

@jpivarski pre-commit is failing after merging with main. Should I wait for it to be fixed before merging?

@agoose77
Copy link
Collaborator

agoose77 commented Feb 7, 2024

@ManasviGoyal if you can hold off on merging, I'll fix these in a separate PR and merge them into main :) Will be about an hour all-in.

@ManasviGoyal
Copy link
Collaborator Author

@ManasviGoyal if you can hold off on merging, I'll fix these in a separate PR and merge them into main :) Will be about an hour all-in.

Sure. Thanks!

@ManasviGoyal ManasviGoyal merged commit dd4753b into main Feb 7, 2024
38 checks passed
@ManasviGoyal ManasviGoyal deleted the ManasviGoyal/add-cumulative-sum-kernels branch February 7, 2024 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gpu Concerns the GPU implementation (backend = "cuda')
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants