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 set functions [Array API] #619

Merged
merged 3 commits into from Jan 8, 2024

Conversation

mtsokol
Copy link
Collaborator

@mtsokol mtsokol commented Jan 4, 2024

Hi @hameerabbasi,

This PR adds four set functions for COO format described in the Array API standard:

  • unique_all
  • unique_counts
  • unique_inverse
  • unique_values

@mtsokol mtsokol self-assigned this Jan 4, 2024
sparse/_coo/common.py Fixed Show fixed Hide fixed
sparse/_coo/common.py Fixed Show fixed Hide fixed
sparse/_coo/common.py Fixed Show fixed Hide fixed
Copy link

codecov bot commented Jan 4, 2024

Codecov Report

Merging #619 (f45a2b0) into master (b8f2717) will decrease coverage by 0.07%.
The diff coverage is 82.50%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #619      +/-   ##
==========================================
- Coverage   90.43%   90.36%   -0.07%     
==========================================
  Files          20       20              
  Lines        3564     3601      +37     
==========================================
+ Hits         3223     3254      +31     
- Misses        341      347       +6     

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.

I'll give this a more in-depth review later, but here are some high-level suggestions.

sparse/tests/test_coo.py Show resolved Hide resolved
sparse/_coo/common.py Outdated Show resolved Hide resolved
@hameerabbasi
Copy link
Collaborator

Okay, so I looked through the standard -- For unique_inverse and unique_all, it doesn't make sufficient sense to implement them, because there's "no better algorithm than dense", please let me know if I'm missing something. The other two, it definitely makes sense to implement.

@mtsokol
Copy link
Collaborator Author

mtsokol commented Jan 5, 2024

Okay, so I looked through the standard -- For unique_inverse and unique_all, it doesn't make sufficient sense to implement them, because there's "no better algorithm than dense", please let me know if I'm missing something. The other two, it definitely makes sense to implement.

For unique_all I agree as indices component requires finding the first occurrence of the fill value, I can make it throw an exception saying that it isn't supported by COO.
For unique_inverse I construct inverse as a DOK and only plug non-fill values so it isn't a dense algorithm I think - but is inverse even useful here?

@mtsokol
Copy link
Collaborator Author

mtsokol commented Jan 7, 2024

I removed unique_all and unique_inverse functions (updated PR's description).

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.

Small changes, otherwise LGTM.

sparse/_coo/common.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 Fixed Show fixed Hide fixed
sparse/_coo/common.py Fixed Show fixed Hide fixed
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.

Last set of comments otherwise LGTM.

sparse/_coo/common.py Outdated Show resolved Hide resolved
sparse/_coo/common.py Outdated Show resolved Hide resolved
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.

Looks great! Thanks for your work on this.

@hameerabbasi hameerabbasi merged commit 925112f into pydata:master Jan 8, 2024
13 checks passed
@mtsokol mtsokol deleted the set-functions branch January 8, 2024 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants