Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

REF: split out dtype-finding in concat_compat #53260

Merged
merged 5 commits into from
May 22, 2023

Conversation

jbrockmendel
Copy link
Member

Split off from #52532

@mroeschke mroeschke added the Refactor Internal refactoring of code label May 16, 2023
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

generally looks good - have left some minor comments, but I'm not really familiar-enough with a lot of what's being used here to be able to merge

"""
from pandas.core.construction import ensure_wrapped_if_datetimelike

to_concat = [ensure_wrapped_if_datetimelike(x) for x in to_concat]
Copy link
Member

Choose a reason for hiding this comment

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

also, could you explain why ensure_wrapped_if_datetimelike and atleast_2d are no longer needed here?

in general, for anything non-trivial, I'd really appreciate a comment explaining why you're making changes, else reviews can really take hours

Copy link
Member Author

Choose a reason for hiding this comment

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

the not single_dtype case is irrelevant bc in that case we find_common_dtype and cast to it. The ensure_wrapped_if_datetimelike is unnecessary bc caller is responsible for ensuring we are wrapped where appropriate. The axis is unnecessary bc the axis=1 cases all go through the lib.dtypes_all_equal path at the top of concat_compat

Copy link
Member

Choose a reason for hiding this comment

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

The axis is unnecessary bc the axis=1 cases all go through the lib.dtypes_all_equal path at the top of concat_compat

Not sure I follow, sorry - in test_append_new_columns, it reaches concat_compat with axis=1 but it doesn't go through the # fastpath! path at the top of concat_compat

Copy link
Member Author

Choose a reason for hiding this comment

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

test_append_new_columns doesnt have any datetime columns, so wouldn't go through _concat_datetime. Should have been clearer thats what i was referring to.

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

Looks fine, no objections to you merging if you're confident about this

@mroeschke mroeschke added this to the 2.1 milestone May 22, 2023
@mroeschke mroeschke merged commit 2f49f73 into pandas-dev:main May 22, 2023
@mroeschke
Copy link
Member

Thanks @jbrockmendel

@jbrockmendel jbrockmendel deleted the ref-concat-compcat branch May 22, 2023 18:20
topper-123 pushed a commit to topper-123/pandas that referenced this pull request May 22, 2023
* REF: split out dtype-finding in concat_compat

* mypy fixup

* fix annotation

* remove unused ignore
Daquisu pushed a commit to Daquisu/pandas that referenced this pull request Jul 8, 2023
* REF: split out dtype-finding in concat_compat

* mypy fixup

* fix annotation

* remove unused ignore
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Refactor Internal refactoring of code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants