Skip to content

ENH: Support mask in unique #48109

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

Merged
merged 13 commits into from
Aug 18, 2022

Conversation

phofl
Copy link
Member

@phofl phofl commented Aug 16, 2022

cc @jorisvandenbossche

@phofl phofl marked this pull request as draft August 16, 2022 10:55
@@ -310,7 +310,7 @@ def _check_object_for_strings(values: np.ndarray) -> str:
# --------------- #


def unique(values):
def unique(values, mask=None):
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if this is a good idea, since this changes the api. A better alternative would probably be a _unique function that can take a mask

Copy link
Member

Choose a reason for hiding this comment

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

agreed

also can annotate mask?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, will do when moving to a private function

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@phofl phofl added Algos Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff NA - MaskedArrays Related to pd.NA and nullable extension arrays labels Aug 16, 2022
@phofl phofl marked this pull request as ready for review August 16, 2022 13:47
@jorisvandenbossche
Copy link
Member

This probably closes #30037

-------
uniques : BaseMaskedArray
"""
uniques, mask = algos._unique(self._data, self._mask)
Copy link
Member

Choose a reason for hiding this comment

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

nitpick, can you rename the function to something non-private, e.g. unique_with_mask

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, done

Copy link
Member

@jorisvandenbossche jorisvandenbossche 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!

@@ -652,10 +697,14 @@ cdef class {{name}}HashTable(HashTable):
Unique values of input, not sorted
labels : ndarray[intp_t] (if return_inverse)
The labels from values to uniques
mask : ndarray[bool], optional
If not None, the mask is used as indicator for missing values
(True = missing, False = valid) instead of `na_value` or
Copy link
Member

Choose a reason for hiding this comment

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

Was this meant to be added to the Parameters section?
(although actually also the return value changes if passing a mask, maybe that can be clarified as well)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes thanks. Moved the mask part up and added a result_mask return value

@mroeschke mroeschke added this to the 1.5 milestone Aug 17, 2022
@phofl
Copy link
Member Author

phofl commented Aug 17, 2022

Would merge tomorrow if no objections

@jorisvandenbossche jorisvandenbossche merged commit 655d9f4 into pandas-dev:main Aug 18, 2022
@jorisvandenbossche
Copy link
Member

Thanks @phofl !

@phofl phofl deleted the enh_support_mask_in_unique branch August 18, 2022 07:29
noatamir pushed a commit to noatamir/pandas that referenced this pull request Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Algos Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff NA - MaskedArrays Related to pd.NA and nullable extension arrays
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ENH: add mask-aware implementation of factorize algos
4 participants