REF: implement NDFrame._from_mgr
State: closed

- [ ] closes #xxxx (Replace xxxx with the GitHub issue number)
- [ ] [Tests added and passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#writing-tests) if fixing a bug or adding a new feature
- [ ] All [code checks passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#pre-commit).
- [ ] Added [type annotations](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#type-hints) to new arguments/methods/functions.
- [ ] Added an entry in the latest `doc/source/whatsnew/vX.X.X.rst` file if fixing a bug or adding a new feature.

cc @phofl @jorisvandenbossche we've discussed this recently-ish but I've lost track of where.

There is a pretty nice performance improvement in ops with small sizes:

```
ser = pd.Series(range(3))

%timeit ser.copy()
17.5 µs ± 580 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)  # <- main
11 µs ± 281 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)  # <- PR
```

Making axes required despite the fact that we don't use them ATM bc that opens up the option of refactoring axes out of the Managers xref #48126.



Labels:
name: Internals
description: Related to non-user accessible pandas implementation

Comments:
Compatibility wise, this can break existing subclass implementations since `_from_mgr` is a new method that they don't yet override. So if we want to do this, should we first introduce the method for some time but while setting it equal to `_constructor`, and then later change the implementation internally?
> should we first introduce the method for some time but while setting it equal to _constructor, and then later change the implementation internally?

Sounds good!
Hmm this is straightforward to do for _sliced_from_mgr and _expanddim_from_mgr, but making _from_mgr itself breaks as long as _from_mgr is a classmethod instead of a method.  abstraction-wise it makes more sense as a classmethod, but i can try making it a method and see if that breaks anything else
How long would you want to keep that for before making a "real" _from_mgr method?  im hoping the answer isn't "until 3.0", since we need a real version to exist before we can deprecate passing Manager to the constructors
> How long would you want to keep that for before making a "real" _from_mgr method? im hoping the answer isn't "until 3.0"

Maybe yes? The problem here is that this is not easy to communicate via a deprecation (how do subclasses get notified that they need to change this, apart from expecting them to read release notes? Is there a way we can somehow signal this?)

> since we need a real version to exist before we can deprecate passing Manager to the constructors

Would it be possible to somehow do this in parallel? If we have a way to signal to the constructor whether to raise the warning or not (eg, when called from via `_constructor`, don't raise a warning, when called via `DataFrame(..)` directly, do raise the warning) 
One possible way would be to suppress that warning in `_constructor`, but that probably has a too high overhead / impact on performance?

> how do subclasses get notified that they need to change this, apart from expecting them to read release notes? Is there a way we can somehow signal this?

Good question.  My working assumption is that informing geopandas gets us 90+% of the way there.  That last few % may need to be the "release notes" approach.

I guess we could implement a `__init_subclass__` method and do some checking/warning there.

> Would it be possible to somehow do this in parallel? If we have a way to signal to the constructor [...]

The natural way to signal the constructor would be a new keyword.  I guess if we make it super-duper explicit that the keyword is not there to stay and users/libraries should never ever use it...

Any thoughts on the classmethod bit?  Making _from_mgr a regular method mostly works, but makes it so we can't call it from other classmethods, in particular from_records.
Would something like this work, if we define our own `DataFrame._constructor` as:

```
@property
def _constructor(self):
    def constructor(*args, **kwargs):
        if first arg is Manager:
            return self._from_mgr(*args, **kwargs) 
        else:
            return DataFrame(*args, **kwargs)
    return constructor
```

That ensures that internally we don't call `DataFrame(..)` with a manager, and thus can deprecate that. While subclasses that override `_constructor` with there subclassed DataFrame will still typically call super init, and thus actually see the warning, which can signal them they need to do something (change `_constructor` and override `_from_mgr`)

> My working assumption is that informing geopandas gets us 90+% of the way there. That last few % may need to be the "release notes" approach.

Are you sure that subclasses that return a class from `_constructor` are not affected / wouldn't need to change anything to automatically get the correct behaviour? 
I assume the future implementation of `_from_mgr` will "work" for such subclasses in the sense that it will return the correct class. But it stil doesn't go through their own custom init, so it might not be fully setting up the subclass instance correctly?

> Any thoughts on the classmethod bit? Making _from_mgr a regular method mostly works, but makes it so we can't call it from other classmethods, in particular from_records.

I didn't fully follow your explanation above. What is the reason that it breaks things if it is a class method? 
Now, currently `_constructor` is a property, which means that the logic inside that can use `self`. So changing it to a class method would limit that.
> Would something like this work, if we define our own DataFrame._constructor as:

That looks plausible.  Will need to check how it affects performance.

> I didn't fully follow your explanation above. What is the reason that it breaks things if it is a class method?
Now, currently _constructor is a property, which means that the logic inside that can use self. So changing it to a class method would limit that.

The most important/relevant place is in DataFrame.from_records (a classmethod) which currently ends with `return cls(mgr)`.  Unless _from_mgr is a classmethod, this cannot be changed to `cls._from_mgr(mgr)`

> But it stil doesn't go through their own custom init, so it might not be fully setting up the subclass instance correctly?

Good point.
> The most important/relevant place is in DataFrame.from_records (a classmethod) which currently ends with `return cls(mgr)`. Unless _from_mgr is a classmethod, this cannot be changed to `cls._from_mgr(mgr)`

Given that `DataFrame.from_records` already has this "problem" for subclasses (going through `cls(..)` and not `_constructor`, and so subclasses that have to customize it would have to override those class methods, which is actually exactly what geopandas does for `from_dict`) , I think it is fine that those class methods continue to work that way. So those could use the manual `obj = cls.__new__(cls)` and then call the non-classmethod mgr init?
> Given that DataFrame.from_records already has this "problem" for subclasses [...]

Fair enough.

Looking at the suggested _constructor implementation I want to double-check.  This PR currently changes a bunch of uses of `self._constructor(...)` to `self._from_mgr(...)`.  Is your suggestion to revert those changes to continue using self._constructor for the interim?
> Is your suggestion to revert those changes to continue using self._constructor for the interim?

Yes, otherwise that would be a breaking change for subclasses that rely on `_constructor` being called in those cases and that don't yet have a custom `_from_mgr`
@jorisvandenbossche thanks for the reminder to take another look at https://github.com/pandas-dev/pandas/pull/52132#issuecomment-1483767546 (BTW that reminder was in the form of a comment on the dev meeting notes. I hit a check-mark there thinking that was akin to a thumbs-up but it made the note disappear.  Just in case there was any ambiguity: the intent was to convey the thumbs-up)

> [Defining _constructor to dispatch to _from_mgr when a Manager is passed] ensures that internally we don't call DataFrame(..) with a manager, and thus can deprecate that [...]

This is clever, and I think would work.  I'm wary bc 1) part of the point of this is performance, which this would hurt, even if just a little, and 2) that conflicts with deprecating non-class _constructor.

> Are you sure that subclasses that return a class from _constructor are not affected / wouldn't need to change anything to automatically get the correct behaviour? [...] But it stil doesn't go through their own custom init, so it might not be fully setting up the subclass instance correctly?

This is definitely possible.  Such subclasses would need to override _from_mgr.

> I didn't fully follow your explanation [of trouble regarding making _from_mgr a classmethod] above. What is the reason that it breaks things if it is a class method?

I think that topic got successfully cleared up, pls let me know if im wrong on this point?
+1 let's just do this already 

tbh we are making mountains of molehills for subclasses - yea we have some support but so what

let's just do this already 

we need velocity not endless discussions about every point 
@jbrockmendel thanks for giving this another look! The google doc comment resolving was correctly interpreted

> This is clever, and I think would work. I'm wary bc 1) part of the point of this is performance, which this would hurt, even if just a little, and 2) that conflicts with deprecating non-class _constructor.

Regarding performance: it's true it requires an extra check of the input type, but that's something that is currently needed in `__init__` as well, so at least it wouldn't regress (it just blocks improving it for some time). 
Regarding the second point: I assume you can understand that I don't mind that conflict ;)

> > Are you sure that subclasses that return a class from _constructor are not affected ...
> 
> This is definitely possible. Such subclasses would need to override _from_mgr.

Yes, I assume so as well. I think the nice thing about my proposal above is that this gives subclasses the time to notice that they need to do this, without directly breaking them. 
Assuming that a subclass overrides `_constructor`, they don't get our version that dispatches to `_from_mgr` (and so don't directly have the issue that our `_from_mgr` wouldn't work correctly for subclasses), but they still use their own version which will typically at some point call the parent init, i.e. calling `DataFrame(*args, **kwargs)`. If we deprecate passing a manager object in this init, they will see those warnings, which can point them to the need to implement `_from_mgr` (the deprecation message can include some details specific towards subclass implementors). 

One annoyance with this approach is that we would still be using `_constructor` internally (to allow for the adaptation period), but so that means that the subclasses also need to do this `if first arg is Manager: ..` check in their own `_constructor`, or still have some custom handling for Managers in their init (to avoid passing it to DataFrame init). 
(now, if they support multiple versions of pandas, they will need to keep some support for accepting managers in their init anyway, so that's maybe not too bad).

> [on being a normal method vs classmethod] I think that topic got successfully cleared up, pls let me know if im wrong on this point?

Just to be sure: the current idea is to make those from_mgr methods normal methods then, and not class methods? (as in the current diff)
This pull request is stale because it has been open for thirty days with no activity. Please [update](https://pandas.pydata.org/pandas-docs/stable/development/contributing.html#updating-your-pull-request) and respond to this comment if you're still interested in working on this.
Another option, instead of short-term having a customized `_constructor` to decide whether to call the main constructor or the from_mgr one (https://github.com/pandas-dev/pandas/pull/52132#issuecomment-1483767546), would be to do that in an already introduced `_from_mgr`. Something like:

```python
    @classmethod
    def _from_mgr(cls, mgr: Manager, axes: list[Index]) -> Self:
        obj = cls.__new__(cls)
        NDFrame.__init__(obj, mgr)
        return obj

    def _constructor_from_mgr(self, mgr):
        if self._constructor is DataFrame._constructor:
            # we are pandas.DataFrame (or a subclass that doesn't override _constructor)
            return self._from_mgr(mgr)
        else:
            return self._constructor(mgr)
```

This would also allow already starting to actually deprecate `DataFrame(mgr)`, and give subclasses a way to avoid the deprecation warning (either by overriding `_constructor_from_mgr`, or by avoiding to pass manager object to `super().__init__` in their constructor).

The two methods could be merged into one (just `_from_mgr`), but having one version that is a class method might be nice, since that also works for subclasses in case they don't need to customize the constructor in the future. 
Not sure how useful that would be, though.

The question is also, for all possible options, what would a typical future implementation of a subclass' `_from_mgr` look like?

Maybe in many cases, you might not actually want to customize something (if you rely on `_metadata`/`__finalize__` to pass through information), in which case you might just want to inherit the pandas one (assuming this sets the class correctly).  
In that case, you could override `_from_mgr` to also do what the upstream pandas version does (using the naming of methods from my last post):

```python
class MySubclassedDataFrame(pd.DataFrame):
    ...

    def _constructor_from_mgr(self, mgr):
        # this will call the class method defined in the parent pandas.DataFrame
        return self._from_mgr(mgr)
```

Does that look correct? (in the future, overriding it that way would be redundant I assume, but it would enable the subclass to make the transition / support multiple pandas versions without getting warnings)

Or you could add a check in your `MySubclassedDataFrame.__init__` for a manager as first argument, and in that case call this `_from_mgr`.
I'm leaning towards a path similar to what @jorisvandenbossche suggested [here](https://github.com/pandas-dev/pandas/pull/52132#issuecomment-1548016785).
Thanks for the update!

> Making axes required despite the fact that we don't use them ATM bc that opens up the option of refactoring axes out of the Managers

Coming back to this: would it also be sufficient to just have `**kwargs` in `_constructor_from_mgr` and pass them through to `_from_mgr` (and documenting that subclasses should also do this when overriding that method). 
That would avoid already having to add `axes=mgr.axes` everywhere (slightly complicating this call everywhere while it is not yet needed), while it should give also the future proofing to add this keyword later on? 

(this assumes that `_constructor_from_mgr` is only used internally, and not by subclasses in methods they override, but I _think_ that should be a correct assumption)

> Coming back to this: would it also be sufficient to just have **kwargs in _constructor_from_mgr and pass them through to _from_mgr (and documenting that subclasses should also do this when overriding that method).

I think that would work.  In general kwargs is a pattern I try to avoid, but I don't care enough to make it a sticking point if you feel strongly about it.
I don't feel super strong about it, but I mostly don't like seeing `axes=mgr.axes` being passed everywhere while that is essentially never used. It can easily give the impression to a contributor that it _is_ used, leading to trying to pass something else intentionally / being confused about this not doing anything.
> I don't feel super strong about it, but I mostly don't like seeing axes=mgr.axes being passed everywhere while that is essentially never used. It can easily give the impression to a contributor that it is used, leading to trying to pass something else intentionally / being confused about this not doing anything.

That's reasonable.  So both the `axes=mgr.axes` and the `**kwargs` options have the bad silently-unused characteristic.  How about I'll remove the keyword from this PR and then we can add it later when it is actually needed.  As long as there isn't a released version where there is a risk of downstream authors using it, that should be safe.
> How about I'll remove the keyword from this PR and then we can add it later when it is actually needed

This turns out to be something of a hassle.  Any chance we can all be OK with the current implementation?
thx @jbrockmendel