Skip to content

GH1396 Add concat compatibility for subclasses of DataFrame#1713

Open
loicdiridollou wants to merge 3 commits intopandas-dev:mainfrom
loicdiridollou:gh1396_dataframe_subclass
Open

GH1396 Add concat compatibility for subclasses of DataFrame#1713
loicdiridollou wants to merge 3 commits intopandas-dev:mainfrom
loicdiridollou:gh1396_dataframe_subclass

Conversation

@loicdiridollou
Copy link
Copy Markdown
Member

  • Closes pd.concat with GeoDataFrames #1396
  • Tests added (Please use assert_type() to assert the type of any return value)
  • If I used AI to develop this pull request, I prompted it to follow AGENTS.md.

@loicdiridollou loicdiridollou requested a review from cmp0xff March 30, 2026 04:17
Comment on lines 69 to 93
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would it make sense to merge these two overloads?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not super clear to me how we could combine both since everything that is NDFrame should go to DataFrame and anything DataFrame compatible should maintain the type. Did you have an idea about how to implement that?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seems to me that DataFrame is the only subclass of NDFrame. What if you use NDFrameT (already in _typing) instead of DataFrameT0?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Series is also a subclass of NDFrame, the issue arises when you have a list of [None, Series, DataFrame] it will give the return type as DataFrame | Series but we want DataFrame (the NDFrameT matches to the type of the union and not the "bigger" type). Not sure if that was clear

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oops, my bad, somehow forgot Series is also a subclass of NDFrame.

If mixing Series and DataFrame gives DataFrame, what if people mix Series and subclasses of DataFrame and complain that pandas-stubs doesn't serve them right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not sure there is a way to do so, I tried with Iterable[DataFrameT0 | NDFrame | None] and it is raising a lot of issues with Unknown. I would say in general that this is not what is intended like a Geopandas and a Series should not be allowed.

@loicdiridollou loicdiridollou force-pushed the gh1396_dataframe_subclass branch from 56faa72 to a2743db Compare April 1, 2026 21:17
@loicdiridollou loicdiridollou requested a review from cmp0xff April 1, 2026 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pd.concat with GeoDataFrames

2 participants