BUG: pandas main setitem mixin

[m-richards]

> obj = cls.__new__(cls) inside pandas _from_mgr is throwing
TypeError: pandas._libs.internals.SetitemMixin.__new__(GeoSeries) is not safe, use GeoSeries.__new__()
is getting thrown in the CI against pandas dev, seems like this corresponds to https://github.com/pandas-dev/pandas/pull/62070/files
but I haven't done a bisect. Might just be an mro thing but I'm not entirely sure.

fyi @jorisvandenbossche, I might have a look but you've probably got a better idea.

[m-richards]

Minimal reproducer is:

from geopandas import GeoSeries
import pandas as pd
pd.Series.__new__(pd.Series) # fine
GeoSeries() # fine
GeoSeries.__new__(GeoSeries) # crashes

and the error message from python itself is a bit garbled.
And it seems to some interaction between the new SetItemMixin in the DataFrame init and the fact we also have multiple inheritance with the GeoPandasBase class meaning this won't instantiate.

If I swap

class GeoSeries(GeoPandasBase, Series):

to

class GeoSeries(Series, GeoPandasBase):

then this doesn't hard crash, but we then don't get the right method resolution order

I notice:

    # override those to avoid inheriting from SetitemMixin (cython generates
    # them by default)
    __reduce__ = object.__reduce__
    __setstate__ = NDFrame.__setstate__

wondering if there's other implied things inherited causing the issue (I would think it would be fine though since we don't directly inherit the mixin).

I also think we have to update our GeoDataFrame.setitem implementation to be in _setitem because we'd be overriding the CoW warning fix here.

[m-richards]

Actual MRE, without geopandas or pandas, just with Cython. Something weird with MRO for cdef classes. Probably a cython bug / limitation? https://github.com/m-richards/cython_mro_reproducer

[jorisvandenbossche]

Ugh, this already gave me a hard time figuring this out in pandas, it was probably wishful thinking that it would not affect subclasses ...
(and thanks for taking a look! although there is probably no easy solution, certainly if it is an issue in cython. Another reason to try to find an alternative solution for pandas-dev/pandas#61368)

[m-richards]

is there are reason the mixin has to be a cdef class? I'm guessing maybe there's a tiny performance boost.
But could the mixin not just be a regular class (defined in cython) along the lines of

cdef void chained_assignment_setitem_tracing(inst, key, value):
    ...
    
class MreSetitemMixin:

    def __setitem__(self, key, value) -> None:
        chained_assignment_setitem_tracing(self, key, value)
        self._setitem(key, value)

    # def __delitem__(self, key) -> None:
    #     self._delitem(key)

calling into a function with the special behaviour?

Though there is a much simpler fix but not general case, we can add

class GeoSeries(GeoPandasBase, Series):
    __new__ = object.__new__

(but that doesn't appear to work if done to the pd.Series, for reasons I don't follow)

[jorisvandenbossche]

is there are reason the mixin has to be a cdef class? I'm guessing maybe there's a tiny performance boost.

Hmm, not sure anymore if I did that intentionally. Using a normal python class fixes it for your reproducer?
(will check if that works in the pandas case)

[jorisvandenbossche]

But unfortunately the fix requires it to be a cdef class .. At least, just removing the cdef causes the specific tests about warnings for the chained assignment to fail.
Looking at the generated C code, a potential difference is in the type of self in the setitem method: a specific struct in the case of the cdef class, but a generic PyObject in case of a python class. And that might influence the way it is called, whether the reference of self is increased first?

[jorisvandenbossche]

It looks like we will be able to revert this on the pandas side: pandas-dev/pandas#63186