Skip to content

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
@ManasviGoyal ManasviGoyal force-pushed the ManasviGoyal/add-cumulative-sum-kernels branch from e591460 to 0299bab Compare January 30, 2024 09:35
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
@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.

4 participants