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

BUG/API: _values_for_factorize/_from_factorized round-trip #32798

Closed
wants to merge 3 commits into from

Conversation

jbrockmendel
Copy link
Member

having this round-trip-ability is necessary for implementing an efficient general-case broadcast_to/tile compat method.

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.

Let's first discuss in the issue what we want

@jreback jreback added the ExtensionArray Extending pandas with custom dtypes or arrays. label Mar 19, 2020
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

lgtm.

@jreback jreback added this to the 1.1 milestone Mar 21, 2020
@jreback
Copy link
Contributor

jreback commented Mar 21, 2020

@jorisvandenbossche objections?

@jorisvandenbossche
Copy link
Member

Yes, see my comment, we should first discuss further in the issue before simply changing

@@ -320,7 +320,8 @@ def _values_for_factorize(self) -> Tuple[np.ndarray, int]:

@classmethod
def _from_factorized(cls, values, original: "BooleanArray") -> "BooleanArray":
return cls._from_sequence(values, dtype=original.dtype)
mask = values == -1
return cls(values.astype(bool), mask)
Copy link
Contributor

Choose a reason for hiding this comment

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

The astype causes a copy, which I think wasn't present before. This could perhaps be a .view('bool') to preserve the previous no-copy behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

changed to .astype(bool, copy=False)

@jbrockmendel
Copy link
Member Author

mothballing

@jbrockmendel jbrockmendel deleted the vff branch November 20, 2021 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ExtensionArray Extending pandas with custom dtypes or arrays.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API: should values_for_factorize and _from_factorized round-trip missing values?
4 participants