Pass method in __finalize__ #33273
Pass method in __finalize__ #33273
Conversation
This passes `method` everywhere we use `__finalize__`. This will be useful for future enhancements buliding on `__finalize__`.
|
maybe a silly question, but for this to be useful, doesnt it entail putting e.g. |
Depends on just how specific the logic is to if method in {"concat", "merge", "align", ...} # methods involving multiple NDFrames:
self.allows_duplicate_labels = all(ndframe.allows_duplicate_labels for ndframe in others)in this case, there's not any logic specific to |
|
Makes sense, thanks for clarifying
…On Fri, Apr 3, 2020 at 1:52 PM Tom Augspurger ***@***.***> wrote:
when that logic would make more sense in _getitem_multilevel?
Depends on just how specific the logic is to _getitem_multilevel. A
motivating use case is my allows_duplicate_labels PR. In that I would do
something like
if method in {"concat", "merge", "align", ...} # methods involving multiple NDFrames:
self.allows_duplicate_labels = all(ndframe.allows_duplicate_labels for ndframe in others)
in this case, there's not any logic specific to concat. But we use the
method to constrain the type of other to Tuple[NDFrame, ...].
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#33273 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB5UM6DL5IZI2DREHHODEN3RKZEBRANCNFSM4L4M5E7A>
.
|
|
Do we want such specific method names? Because this is kind of "public" interface, we should maybe try to limit to a set of "categories" (but I didn't really think about whether that defeats the purpose of it or not) |
pandas/core/generic.py
Outdated
| result = pd.Series(values, index=self.index, dtype=self.dtype).__finalize__(self) | ||
| result = pd.Series(values, index=self.index, dtype=self.dtype).__finalize__( | ||
| self, method="_single_replace" | ||
| ) |
There was a problem hiding this comment.
can this be broken into two lines? probably also should use _constructor instead of pd.Series
|
also needs a whatsnew note |
|
For
and
Do we make any claim that I'm OK with these specific names and making |
Looking at the docs, it's indeed not mentioned in the docs on subclassing. But personally, I always assumed this is part of our sublcassing API. In any case, geopandas is using it for that reason (so that's probably the reason I was assuming that ;)). |
I was also assuming that, since it does interact with |
|
Regarding part of our subclassing API or not: since we actually don't use @TomAugspurger is it also your goal that external users can implement similar metadata propagation logic as you are experimenting for the allow_duplicate_labels? (I seem to remember we were discussing this at some point I think) |
I'm going back and forth on this. I have a few ideas for what an API would look like, but I'm having trouble deciding what will work well in practice. If I had to guess, I'll propose handling |
|
w/r/t the private/public/experimental aspect, the only opinion i have is that we retain flexibility with this (and not need to wait until 2.0 if minds are changed) |
|
I think this PR is compatible with that. The only "API change" is a clarification that the actual |
|
We should still guarantee some stability for the places where we already specify methods, though. |
|
That makes me hesitant to include I'd rather work with subclassers to find what they're relying on. But since we're in an API gray zone that may be hard. |
|
In an effort to unstick things, I've removed the addition of I opened #33338 as a general discussion about |
|
thanks @TomAugspurger nice improvement |
This passes
methodeverywhere we use__finalize__. I'm trying to get a better sense forI haven't called it anywhere new yet (followup PR with that coming though).