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

API: Add sort and take functions for COO format #627

Merged
merged 3 commits into from
Jan 16, 2024

Conversation

mtsokol
Copy link
Collaborator

@mtsokol mtsokol commented Jan 12, 2024

Hi @hameerabbasi,

This PR adds sort and take Array API functions for the COO format to the main namespace.
sort function, where looping happens, is numba jitted.

@mtsokol mtsokol self-assigned this Jan 12, 2024
Copy link

codecov bot commented Jan 12, 2024

Codecov Report

Merging #627 (5bd29ad) into main (82fb0d5) will decrease coverage by 0.38%.
The diff coverage is 58.57%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #627      +/-   ##
==========================================
- Coverage   90.57%   90.20%   -0.38%     
==========================================
  Files          20       20              
  Lines        3618     3654      +36     
==========================================
+ Hits         3277     3296      +19     
- Misses        341      358      +17     

sparse/_coo/common.py Fixed Show fixed Hide fixed
@mtsokol mtsokol force-pushed the sparse-sort-and-take branch from 33da5ff to ff2084f Compare January 12, 2024 15:40
@mtsokol mtsokol added the enhancement Indicates new feature requests label Jan 12, 2024
Copy link
Collaborator

@hameerabbasi hameerabbasi left a comment

Choose a reason for hiding this comment

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

Great work here! I feel the algorithm for sort could use some improvement. I've left a guide on what to do in the comments, but feel free to ask questions if needed.

sparse/__init__.py Show resolved Hide resolved
sparse/_coo/common.py Outdated Show resolved Hide resolved
sparse/_coo/common.py Outdated Show resolved Hide resolved
sparse/_coo/common.py Outdated Show resolved Hide resolved
sparse/_coo/common.py Outdated Show resolved Hide resolved
sparse/_coo/common.py Outdated Show resolved Hide resolved
@mtsokol mtsokol force-pushed the sparse-sort-and-take branch from 6d0591a to 1b5662d Compare January 16, 2024 15:28
@mtsokol mtsokol force-pushed the sparse-sort-and-take branch 2 times, most recently from 9360efc to af4ea95 Compare January 16, 2024 15:39
sparse/_coo/common.py Dismissed Show dismissed Hide dismissed
sparse/_coo/common.py Dismissed Show dismissed Hide dismissed
sparse/_coo/common.py Dismissed Show dismissed Hide dismissed
Copy link
Collaborator

@hameerabbasi hameerabbasi left a comment

Choose a reason for hiding this comment

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

A couple of comments to ensure better testing.

sparse/tests/test_coo.py Show resolved Hide resolved
sparse/tests/test_coo.py Show resolved Hide resolved
@mtsokol mtsokol force-pushed the sparse-sort-and-take branch from af4ea95 to 16cbe86 Compare January 16, 2024 17:15
sparse/_coo/common.py Dismissed Show dismissed Hide dismissed
sparse/_coo/common.py Dismissed Show dismissed Hide dismissed
@mtsokol mtsokol requested a review from hameerabbasi January 16, 2024 17:20
@mtsokol
Copy link
Collaborator Author

mtsokol commented Jan 16, 2024

Ok, I think I addressed all comments - the PR is ready for another review.

Copy link
Collaborator

@hameerabbasi hameerabbasi left a comment

Choose a reason for hiding this comment

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

LGTM, feel free to hit the button. 😄

@mtsokol
Copy link
Collaborator Author

mtsokol commented Jan 16, 2024

LGTM, feel free to hit the button. 😄

Thanks! Unfortunately I see: "You’re not authorized to merge this pull request."

@hameerabbasi
Copy link
Collaborator

LGTM, feel free to hit the button. 😄

Thanks! Unfortunately I see: "You’re not authorized to merge this pull request."

What about now?

@mtsokol mtsokol merged commit 16b46cd into pydata:main Jan 16, 2024
13 checks passed
@mtsokol mtsokol deleted the sparse-sort-and-take branch January 16, 2024 17:48
@mtsokol
Copy link
Collaborator Author

mtsokol commented Jan 16, 2024

Merged! Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Indicates new feature requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants