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

Closes #2462: Categorical hashing #2487

Merged

Conversation

stress-tess
Copy link
Member

This PR (closes #2462) adds categorical.hash() / ak.hash(categorical) and tests for this functionality.

@stress-tess stress-tess marked this pull request as draft June 6, 2023 16:16
@stress-tess stress-tess force-pushed the 2462_categorical_hashing branch 2 times, most recently from aafa8a7 to f3dbd80 Compare June 6, 2023 17:12
@stress-tess stress-tess marked this pull request as ready for review June 6, 2023 17:17
Copy link
Contributor

@Ethan-DeBandi99 Ethan-DeBandi99 left a comment

Choose a reason for hiding this comment

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

It looks like there was a lot of # type: ignores added in places I would not have expected. Was there an issue with multiple forward refs that lead to this? It seems odd to me that Categorical is able to be referenced directly where SegArray is referenced as a forward ref. Maybe I am missing something, but maybe we could have a bit of discussion moving forward.

Overall, most the comments are simply suggestions. I do think it would be good to limit how much technical debt is being added, but understand waiting on the JSON return updates.

arkouda/categorical.py Outdated Show resolved Hide resolved
arkouda/numeric.py Outdated Show resolved Hide resolved
src/HashMsg.chpl Outdated Show resolved Hide resolved
src/HashMsg.chpl Outdated Show resolved Hide resolved
src/HashMsg.chpl Outdated Show resolved Hide resolved
Pierce Hayes added 2 commits June 7, 2023 18:43
This PR (closes Bears-R-Us#2462) adds `categorical.hash()` / `ak.hash(categorical)` and tests for this functionality.
arkouda/numeric.py Outdated Show resolved Hide resolved
Copy link
Contributor

@jaketrookman jaketrookman left a comment

Choose a reason for hiding this comment

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

All looks good to me

@Ethan-DeBandi99 Ethan-DeBandi99 added this pull request to the merge queue Jun 12, 2023
Merged via the queue into Bears-R-Us:master with commit 2314d1f Jun 12, 2023
@stress-tess stress-tess deleted the 2462_categorical_hashing branch June 12, 2023 19:55
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.

Hashing for Categoricals
3 participants