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.
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: Fix concat of frames with extension types (no reindexed columns) #34339
BUG: Fix concat of frames with extension types (no reindexed columns) #34339
Changes from 10 commits
e130060
ea33cfd
f83338a
5822694
3d32bc8
951e356
d269229
63a6570
e731802
d8faef5
2e1f1a6
94fe89d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I find it odd that the logic is now split here and concat_compat. I would strongly encourage to put this logic there.
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.
Having this here actually ensures a clear separation of concerns between
dtypes/concat.py
andinternals/concat.py
.The
concat_compat
fromdtypes/concat.py
only needs to worry about concatting 1D arrays (for the case of ExtensionArrays, to be clear), and the code here in internals deals with ensuring the result is the proper dimension depending on how it is put in the BlockManager.This keeps this BlockManager-related logic inside the internals, and ensures that
concat_compat
/dtypes/concat.py
doesn't need to care about internals-specific details.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.
my point is you should have a concat_ea which you call here (right near concat_compat), otherwise the logic is scatttered. the *dispatching) logic is fine, the problem is the operational logic does not belong here. (alternative is to pass anotherr arg to concat_compat to do what you are doing 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.
If there is a different
concat_ea
, then at each location whereconcat_compat
is called, we would need to do aif any_ea: concat_ea(..); else: concat_compat(...)
What do you mean exactly with the "operational logic does not belong here". What part of the code below is the "operational" logic?
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.
every bit you added is operational logic.
pls do something like 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.
this is moving L324-330. As I have said before you are mixing 2 types of logic here by adding 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.
To make sure I understand you correctly. You want to see the following function be put in
dtypes/concat.py
:and called her. Is that correct?
I can do that if it ends this discussion, but just to note: this would create a function in
dtypes/concat.py
with logic tied to the internals (reshaping to conform to block's dimenions is something only needs to be known to the internals code) and that would only be used in the internals (here, ininternals/concat.py
)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 this is exactly inline with concat_dateime and concat_compat. I am surprised that you think this should be anywhere else.
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.
i agree with Joris; internals-specific logic belongs in internals
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.
i guess
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.
this change is to ensure DatetimeBlock and DatetimeTZBlock don't incorrectly take this "uniform join units" code path