API/TST: Call __finalize__ in more places #33379
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Progress towards pandas-dev#28283. This adds tests that ensures `NDFrame.__finalize__` is called in more places. Thus far I've added tests for anything that meets the following rule: > Pandas calls `NDFrame.__finalize__` on any NDFrame method that returns > another NDFrame. I think that given the generality of `__finalize__`, making any kind of list of which methods should call it is going to be somewhat arbitrary. That rule errs on the side of calling it too often, which I think is my preference.
|
CI has finished here, just hasn't updated on the UI. On the call yesterday, people generally seemed OK with the rule that methods returning NDFrames should call Does anyone have issues with the structure of the If so, I'll start to work on the xfails (methods that don't yet call |
|
nice work @TomAugspurger |
Comment on lines
+575
to
+576
| if annotate in {"left", "both"} and not isinstance(right, int): | ||
| right.attrs = {"a": 1} |
There was a problem hiding this comment.
Hi @TomAugspurger - I don't suppose you remember, but was this meant to be
if annotate in {"right", "both"} and not isinstance(right, int):
right.attrs = {"a": 1}?
EDIT so sorry, just noticed you'd confirmed this in #34373 (comment) . Please ignore my comment, sorry for the noise
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Progress towards #28283. This
adds tests that ensures
NDFrame.__finalize__is called in more places.Thus far I've added tests for anything that meets the following rule:
I think that given the generality of
__finalize__, making any kind oflist of which methods should call it is going to be somewhat arbitrary.
That rule errs on the side of calling it too often, which I think is my
preference.
For reviewers I think the two most helpful pieces would be
NDFrame -> NDFrameshould call__finalize__?If you don't agree with the general rule, then we'll need to go through method by method and determine whether that method should call
__finalize__.Once we have agreement that the tests I've added accurately reflect the desired outcome, this can be merged and we can fix
xfailsin smaller batches.