-
-
Notifications
You must be signed in to change notification settings - Fork 18.2k
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
REF: implement EA.pad_or_backfill #54001
REF: implement EA.pad_or_backfill #54001
Conversation
@mroeschke help? i dont understand what this is asking for |
@MarcoGorelli any idea whats going on with the docbuild? i have a premonition its going to be really obvious as soon as someone points it out to me |
c951419
to
777fb83
Compare
My guess is that you need to add this new method to doc/source/reference/extensions.rst ? |
That solved it, thanks |
@@ -281,6 +281,16 @@ def convert_values(param): | |||
def value_counts(self, dropna: bool = True): | |||
return value_counts(self.to_numpy(), dropna=dropna) | |||
|
|||
# We override fillna here to simulate a 3rd party EA that has done so. This |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we can remove this in 3.0 when the deprecation is enforced? (if so can we add a note)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I'll add that to my next Assorted branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, just got minor comments
pandas/core/arrays/sparse/array.py
Outdated
limit_area: Literal["inside", "outside"] | None = None, | ||
copy: bool = True, | ||
) -> Self: | ||
msg = "pad_or_backfill with 'method' requires high memory usage." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'method'
is required here? why not just "pad_or_backfill for sparse arrays requires high memory usage."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm i think this warning would be unnecessary if we move SparseArray to use the new get_fill_indexer
implementation. ill do that as a separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated+green
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might need another merge after recent CI changes
I'd suggest adding a little example here too, so as to not add even more to the list of exclusions (which we're hoping to remove next week)
ci/code_checks.sh
Outdated
@@ -82,6 +82,7 @@ if [[ -z "$CHECK" || "$CHECK" == "docstrings" ]]; then | |||
pandas.api.extensions.ExtensionArray._reduce \ | |||
pandas.api.extensions.ExtensionArray._values_for_factorize \ | |||
pandas.api.extensions.ExtensionArray.interpolate \ | |||
pandas.api.extensions.ExtensionArray.pad_or_backfill \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add a little example while we're here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one! Ok to merge imo
Thanks @jbrockmendel |
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.Sits on top of #53989.
The deprecation is a little bit awkward. If a 3rd party EA has implement fillna but not pad_or_backfill, we warn and call that. DecimalArray tests that deprecation.
The "method" paths for our existing EA.fillna methods could be ripped out as they are no longer used. I left them untouched for now.
In a follow-up, can de-duplicate Block.pad_or_backfill with EABlock.pad_or_backfill.