REF: simplify _from_mgr constructors, use in more places #53871
Conversation
mroeschke
added
Refactor
|
Thanks @jbrockmendel |
| if self._constructor is Series: | ||
| # we are pandas.Series (or a subclass that doesn't override _constructor) | ||
| return self._from_mgr(mgr, axes=axes) | ||
| ser = self._from_mgr(mgr, axes=axes) |
There was a problem hiding this comment.
@jbrockmendel this changes the logic from what we discussed in #52132, and I am afraid this isn't exactly equivalent for subclasses implementing _constructor.
(before, we passed the Manager object to _constructor, while with this PR we pass a Series object)
Now, what was the rationale for changing this?
The PR title says "simplify _from_mgr constructors", but don't see how this is a simplification here? (it actually got longer, and added _name handling)
There was a problem hiding this comment.
IIRC i went to use the new constructor_from_mgr in more places and found that broke some tests, and the edits here were the best option I found to keep things working (in particular pinning ser._name)
I also very much like that in this version subclasses don't have to worry about handling Manager objects, just Series/DataFrame objects, which they should already be able to handle.
There was a problem hiding this comment.
Can you explain why or when the pinning of ser._name is needed? (which case is it solving?)
In geopandas, it is a feature that we can do something different for blockmanager vs generic array-like (we use it to decide whether we want to try to infer objects being passed or not). It also adds some overhead to go through the constructor again with a dataframe/series.
There was a problem hiding this comment.
Can you explain why or when the pinning of ser._name is needed? (which case is it solving?)
My recollection is that without that, when I changed the usages to use constructor_from_mgr, a handful of tests broke with AttributeError bc _name was not defined.
Largely split off from #52419