Repository: pandas-dev/pandas
Pull request #54922 (2023-09-01 08:52:29)

REGR: Restore _constructor_from_mgr to pass manager object to constructor
See https://github.com/pandas-dev/pandas/pull/53871#discussion_r1251901671 for context. This reverts the refactoring changes of the `_constructor_.._from_mgr` methods of https://github.com/pandas-dev/pandas/pull/53871 to the state of the original PR https://github.com/pandas-dev/pandas/pull/52132 that added those new methods. 

This _does_ keep the other changes of that PR (i.e. using `_constructor_from_mgr` in more places). @jbrockmendel mentioned he got some AttributeErrors related to `_name` not yet being defined, but at the moment the tests seem to pass anyway.

Closes https://github.com/pandas-dev/pandas/issues/55120

jbrockmendel: The motivation is that geopandas does something involving inference on object dtypes differently if it gets a Manager object?  Can you elaborate on that?
lithomas1: Bumping off the milestone, assuming the release happens tomorrow.
lithomas1: CI is failing, bumping off the release.
jorisvandenbossche: @lithomas1 if you bump something (at least for regressions), can you please bump it to the _next_ bug-fix milestone (although I assume in general for PRs that's not a problem, as long as you bump issues to the next milestone instead of removing the milestone. So I should have opened an issue for this one ;))
jorisvandenbossche: > The motivation is that geopandas does something involving inference on object dtypes differently if it gets a Manager object? Can you elaborate on that?

@jbrockmendel yes, whenever we get a manager, we don't do _any_ inference, while if we get object dtype array, we do some inference (and then potentially fallback to DataFrame instead of GeoDataFrame in case there are no geometries). See also my geopandas PR related to this: https://github.com/geopandas/geopandas/pull/3046

But that's geopandas specific (and something we can handle by overriding the constructor from mgr), while this PR actually addresses a more general issue: with your change in https://github.com/pandas-dev/pandas/pull/53871, creating a Series or DataFrame object with `self._from_mgr` and passing that to `_constructor`, the _consequence_ of this is that we are now passing a non-fully initialized object to the subclass constructor. And there is no guarantee that the subclass constructor works with such partial objects. 

Essentially, this is the same as https://github.com/pandas-dev/pandas/pull/55607, where `Series._from_mgr` currently returns an non-fully initialized Series object (there is an attribute missing, so some methods don't work). Similarly for subclasses, when this is created with `_from_mgr`, it didn't go through the subclass' `__init__` function, and does the subclass object might not be fully correctly be set up.

I think, when passing (subclassed) DataFrame/Series objects to the public constructor (`__init__`), one should always be able to assume that this are valid, fully-initialized objects. 

I added a generic test that demonstrates this (`test_constructor_with_metadata` in `frame/test_subclass.py`). This test works on 2.0, but fails on 2.1.1.
jorisvandenbossche: I am adding a bunch of filterwarnings here, while of course we should fix those cases properly. But I decided to go with the filtering here, because firstly those warnings don't yet exist on 2.1.x and so properly fixing them isn't need to backport. And secondly, we then also have a bit more time to figure out the best way we want to have subclasses deal with this (eg Brock's comment at https://github.com/geopandas/geopandas/pull/3046/files#r1371974580 might mean we want to tweak a bit the way that subclasses interact with this). I am also not sure whether we actually want to let all subclasses deal with this, instead of rather doing it as a breaking change in eg 3.0 (or let subclasses handle a mgr in their main constructor)
jorisvandenbossche: To illustrate the issue with our own Series:

```
In [4]: s = pd.Series([1, 2, 3], name="test")

In [5]: pd.Series(pd.Series._from_mgr(s._mgr, s._mgr.axes))
...
AttributeError: 'Series' object has no attribute '_name'
```

So our Series `__init__` doesn't work with partially-initialized Series object. We solve that on our side by adding `s._name` inside `Series._constructor_from_mgr`, but subclasses can also have this kind of attributes that have to be added to be able to work properly. And so those subclasses would need to override `_from_mgr` or `_constructor_from_mgr` to do that (and that's something we need to document, and decide how to move towards that, instead of already breaking that in 2.1.x)
jbrockmendel: no objection to getting this in for 2.1.x, can figure out a longer-term plan later

Patch:
diff --git a/doc/source/whatsnew/v2.1.2.rst b/doc/source/whatsnew/v2.1.2.rst
index 1d2d89e7ea..a59cadde42 100644
--- a/doc/source/whatsnew/v2.1.2.rst
+++ b/doc/source/whatsnew/v2.1.2.rst
@@ -29,6 +29,7 @@ Fixed regressions
 - Fixed performance regression with wide DataFrames, typically involving methods where all columns were accessed individually (:issue:`55256`, :issue:`55245`)
 - Fixed regression in :func:`merge_asof` raising ``TypeError`` for ``by`` with datetime and timedelta dtypes (:issue:`55453`)
 - Fixed regression in :meth:`DataFrame.to_sql` not roundtripping datetime columns correctly for sqlite when using ``detect_types`` (:issue:`55554`)
+- Fixed regression in construction of certain DataFrame or Series subclasses (:issue:`54922`)
 
 .. ---------------------------------------------------------------------------
 .. _whatsnew_212.bug_fixes:
diff --git a/pandas/core/frame.py b/pandas/core/frame.py
index 838792ceb4..c8141305e0 100644
--- a/pandas/core/frame.py
+++ b/pandas/core/frame.py
@@ -643,13 +643,12 @@ class DataFrame(NDFrame, OpsMixin):
         return DataFrame
 
     def _constructor_from_mgr(self, mgr, axes):
-        df = self._from_mgr(mgr, axes=axes)
-        if type(self) is DataFrame:
-            # fastpath avoiding constructor call
-            return df
+        if self._constructor is DataFrame:
+            # we are pandas.DataFrame (or a subclass that doesn't override _constructor)
+            return self._from_mgr(mgr, axes=axes)
         else:
             assert axes is mgr.axes
-            return self._constructor(df, copy=False)
+            return self._constructor(mgr)
 
     _constructor_sliced: Callable[..., Series] = Series
 
@@ -657,13 +656,12 @@ class DataFrame(NDFrame, OpsMixin):
         return Series._from_mgr(mgr, axes)
 
     def _constructor_sliced_from_mgr(self, mgr, axes):
-        ser = self._sliced_from_mgr(mgr, axes=axes)
-        ser._name = None  # caller is responsible for setting real name
-        if type(self) is DataFrame:
-            # fastpath avoiding constructor call
+        if self._constructor_sliced is Series:
+            ser = self._sliced_from_mgr(mgr, axes)
+            ser._name = None  # caller is responsible for setting real name
             return ser
         assert axes is mgr.axes
-        return self._constructor_sliced(ser, copy=False)
+        return self._constructor_sliced(mgr)
 
     # ----------------------------------------------------------------------
     # Constructors
diff --git a/pandas/core/series.py b/pandas/core/series.py
index c6625eefa2..c5f622a113 100644
--- a/pandas/core/series.py
+++ b/pandas/core/series.py
@@ -632,14 +632,14 @@ class Series(base.IndexOpsMixin, NDFrame):  # type: ignore[misc]
         return Series
 
     def _constructor_from_mgr(self, mgr, axes):
-        ser = self._from_mgr(mgr, axes=axes)
-        ser._name = None  # caller is responsible for setting real name
-        if type(self) is Series:
-            # fastpath avoiding constructor call
+        if self._constructor is Series:
+            # we are pandas.Series (or a subclass that doesn't override _constructor)
+            ser = self._from_mgr(mgr, axes=axes)
+            ser._name = None  # caller is responsible for setting real name
             return ser
         else:
             assert axes is mgr.axes
-            return self._constructor(ser, copy=False)
+            return self._constructor(mgr)
 
     @property
     def _constructor_expanddim(self) -> Callable[..., DataFrame]:
@@ -657,10 +657,12 @@ class Series(base.IndexOpsMixin, NDFrame):  # type: ignore[misc]
         return DataFrame._from_mgr(mgr, axes=mgr.axes)
 
     def _constructor_expanddim_from_mgr(self, mgr, axes):
-        df = self._expanddim_from_mgr(mgr, axes)
-        if type(self) is Series:
-            return df
-        return self._constructor_expanddim(df, copy=False)
+        from pandas.core.frame import DataFrame
+
+        if self._constructor_expanddim is DataFrame:
+            return self._expanddim_from_mgr(mgr, axes)
+        assert axes is mgr.axes
+        return self._constructor_expanddim(mgr)
 
     # types
     @property


Test patch:
diff --git a/pandas/tests/frame/test_arithmetic.py b/pandas/tests/frame/test_arithmetic.py
index 1d1a4dbe83..c59edbae05 100644
--- a/pandas/tests/frame/test_arithmetic.py
+++ b/pandas/tests/frame/test_arithmetic.py
@@ -2083,6 +2083,9 @@ def test_frame_sub_nullable_int(any_int_ea_dtype):
     tm.assert_frame_equal(result, expected)
 
 
+@pytest.mark.filterwarnings(
+    "ignore:Passing a BlockManager|Passing a SingleBlockManager:DeprecationWarning"
+)
 def test_frame_op_subclass_nonclass_constructor():
     # GH#43201 subclass._constructor is a function, not the subclass itself
 
diff --git a/pandas/tests/frame/test_subclass.py b/pandas/tests/frame/test_subclass.py
index 37ca52eba6..8e657f197c 100644
--- a/pandas/tests/frame/test_subclass.py
+++ b/pandas/tests/frame/test_subclass.py
@@ -10,6 +10,10 @@ from pandas import (
 )
 import pandas._testing as tm
 
+pytestmark = pytest.mark.filterwarnings(
+    "ignore:Passing a BlockManager|Passing a SingleBlockManager:DeprecationWarning"
+)
+
 
 @pytest.fixture()
 def gpd_style_subclass_df():
@@ -734,8 +738,36 @@ class TestDataFrameSubclassing:
         # https://github.com/pandas-dev/pandas/pull/46018
         df = tm.SubclassedDataFrame({"A": [0, 1, 2]})
         msg = "The 'method' keyword in SubclassedDataFrame.replace is deprecated"
-        with tm.assert_produces_warning(FutureWarning, match=msg):
+        with tm.assert_produces_warning(
+            FutureWarning, match=msg, raise_on_extra_warnings=False
+        ):
             result = df.replace([1, 2], method="ffill")
         expected = tm.SubclassedDataFrame({"A": [0, 0, 0]})
         assert isinstance(result, tm.SubclassedDataFrame)
         tm.assert_frame_equal(result, expected)
+
+
+class MySubclassWithMetadata(DataFrame):
+    _metadata = ["my_metadata"]
+
+    def __init__(self, *args, **kwargs) -> None:
+        super().__init__(*args, **kwargs)
+
+        my_metadata = kwargs.pop("my_metadata", None)
+        if args and isinstance(args[0], MySubclassWithMetadata):
+            my_metadata = args[0].my_metadata  # type: ignore[has-type]
+        self.my_metadata = my_metadata
+
+    @property
+    def _constructor(self):
+        return MySubclassWithMetadata
+
+
+def test_constructor_with_metadata():
+    # https://github.com/pandas-dev/pandas/pull/54922
+    # https://github.com/pandas-dev/pandas/issues/55120
+    df = MySubclassWithMetadata(
+        np.random.default_rng(2).random((5, 3)), columns=["A", "B", "C"]
+    )
+    subset = df[["A", "B"]]
+    assert isinstance(subset, MySubclassWithMetadata)
diff --git a/pandas/tests/groupby/test_groupby_subclass.py b/pandas/tests/groupby/test_groupby_subclass.py
index 601e67bbca..0068f642a4 100644
--- a/pandas/tests/groupby/test_groupby_subclass.py
+++ b/pandas/tests/groupby/test_groupby_subclass.py
@@ -11,6 +11,10 @@ from pandas import (
 import pandas._testing as tm
 from pandas.tests.groupby import get_groupby_method_args
 
+pytestmark = pytest.mark.filterwarnings(
+    "ignore:Passing a BlockManager|Passing a SingleBlockManager:DeprecationWarning"
+)
+
 
 @pytest.mark.parametrize(
     "obj",
@@ -64,7 +68,9 @@ def test_groupby_preserves_metadata():
         return group.testattr
 
     msg = "DataFrameGroupBy.apply operated on the grouping columns"
-    with tm.assert_produces_warning(FutureWarning, match=msg):
+    with tm.assert_produces_warning(
+        FutureWarning, match=msg, raise_on_extra_warnings=False
+    ):
         result = custom_df.groupby("c").apply(func)
     expected = tm.SubclassedSeries(["hello"] * 3, index=Index([7, 8, 9], name="c"))
     tm.assert_series_equal(result, expected)
@@ -104,6 +110,8 @@ def test_groupby_resample_preserves_subclass(obj):
 
     # Confirm groupby.resample() preserves dataframe type
     msg = "DataFrameGroupBy.resample operated on the grouping columns"
-    with tm.assert_produces_warning(FutureWarning, match=msg):
+    with tm.assert_produces_warning(
+        FutureWarning, match=msg, raise_on_extra_warnings=False
+    ):
         result = df.groupby("Buyer").resample("5D").sum()
     assert isinstance(result, obj)
diff --git a/pandas/tests/reshape/concat/test_concat.py b/pandas/tests/reshape/concat/test_concat.py
index 1310580108..216422bc12 100644
--- a/pandas/tests/reshape/concat/test_concat.py
+++ b/pandas/tests/reshape/concat/test_concat.py
@@ -591,6 +591,9 @@ def test_duplicate_keys_same_frame():
     tm.assert_frame_equal(result, expected)
 
 
+@pytest.mark.filterwarnings(
+    "ignore:Passing a BlockManager|Passing a SingleBlockManager:DeprecationWarning"
+)
 @pytest.mark.parametrize(
     "obj",
     [
diff --git a/pandas/tests/reshape/merge/test_merge.py b/pandas/tests/reshape/merge/test_merge.py
index 4d779349b5..495fb4536f 100644
--- a/pandas/tests/reshape/merge/test_merge.py
+++ b/pandas/tests/reshape/merge/test_merge.py
@@ -688,6 +688,9 @@ class TestMerge:
         )[["i1", "i2", "i1_", "i3"]]
         tm.assert_frame_equal(result, expected)
 
+    @pytest.mark.filterwarnings(
+        "ignore:Passing a BlockManager|Passing a SingleBlockManager:DeprecationWarning"
+    )
     def test_merge_type(self, df, df2):
         class NotADataFrame(DataFrame):
             @property
diff --git a/pandas/tests/reshape/merge/test_merge_ordered.py b/pandas/tests/reshape/merge/test_merge_ordered.py
index 1d0d4e3eb5..cfb4e92fb4 100644
--- a/pandas/tests/reshape/merge/test_merge_ordered.py
+++ b/pandas/tests/reshape/merge/test_merge_ordered.py
@@ -70,6 +70,9 @@ class TestMergeOrdered:
         result = merge_ordered(left, right, on="key", left_by="group")
         assert result["group"].notna().all()
 
+    @pytest.mark.filterwarnings(
+        "ignore:Passing a BlockManager|Passing a SingleBlockManager:DeprecationWarning"
+    )
     def test_merge_type(self, left, right):
         class NotADataFrame(DataFrame):
             @property
diff --git a/pandas/tests/series/methods/test_to_frame.py b/pandas/tests/series/methods/test_to_frame.py
index 01e547aa34..0eadf696b3 100644
--- a/pandas/tests/series/methods/test_to_frame.py
+++ b/pandas/tests/series/methods/test_to_frame.py
@@ -1,3 +1,5 @@
+import pytest
+
 from pandas import (
     DataFrame,
     Index,
@@ -40,6 +42,9 @@ class TestToFrame:
         )
         tm.assert_frame_equal(rs, xp)
 
+    @pytest.mark.filterwarnings(
+        "ignore:Passing a BlockManager|Passing a SingleBlockManager:DeprecationWarning"
+    )
     def test_to_frame_expanddim(self):
         # GH#9762
 
diff --git a/pandas/tests/series/test_subclass.py b/pandas/tests/series/test_subclass.py
index a3550c6de6..c2d5afcf88 100644
--- a/pandas/tests/series/test_subclass.py
+++ b/pandas/tests/series/test_subclass.py
@@ -4,6 +4,10 @@ import pytest
 import pandas as pd
 import pandas._testing as tm
 
+pytestmark = pytest.mark.filterwarnings(
+    "ignore:Passing a BlockManager|Passing a SingleBlockManager:DeprecationWarning"
+)
+
 
 class TestSeriesSubclassing:
     @pytest.mark.parametrize(