Discussion regarding `kwargs` about simpeg HOT 11 OPEN

You're right, it would be very complicated to remove **kwargs everywhere. I don't think we should tackle this all at once. But I think we should still discuss this topic for having a decision towards new code, refactors, etc.

I agree that removing **kwargs from public classes is a great way to start.

from simpeg.

As far as documenting the kwargs inherited from parents, I know matplotlib has automatic generators for their inherited portions, https://github.com/matplotlib/matplotlib/blob/main/lib/matplotlib/_docstring.py#L78, that we could take some inspiration from.

from simpeg.

@jcapriot made a point during today's meeting regarding the second issue I mention. That is to add a super().__init__(**kwargs) in the topmost parent class. This way, instantiation would fail if there's any residual kwarg that reaches that class.

For example:

class ParentOne:
    def __init__(self, x, **kwargs):
        super().__init__(**kwargs)
        self.x = x


class ChildOne(ParentOne):
    def __init__(self, x, y, **kwargs):
        super().__init__(x, **kwargs)
        self.y = y

If we try to instantiate the ChildOne class with an extra argument, we get:

ChildOne(1, 2, argument=3)

TypeError                                 Traceback (most recent call last)
Cell In[2], line 1
----> 1 ChildOne(1, 2, argument=3)

File ~/tmp/sandbox/testing.py:9, in ChildOne.__init__(self, x, y, **kwargs)
      8 def __init__(self, x, y, **kwargs):
----> 9     super().__init__(x, **kwargs)
     10     self.y = y

File ~/tmp/sandbox/testing.py:3, in ParentOne.__init__(self, x, **kwargs)
      2 def __init__(self, x, **kwargs):
----> 3     super().__init__(**kwargs)
      4     self.x = x

TypeError: object.__init__() takes exactly one argument (the instance to initialize)

On the other hand, if we avoid kwargs, we can define these two classes:

class ParentTwo:
    def __init__(self, x):
        self.x = x


class ChildTwo(ParentTwo):
    def __init__(self, x, y):
        super().__init__(x)
        self.y = y

And if we instantiate the ChildTwo with an extra non-valid argument, we get:

ChildTwo(1, 2, argument=3)

TypeError                                 Traceback (most recent call last)
Cell In[3], line 1
----> 1 ChildTwo(1, 2, argument=3)

TypeError: ChildTwo.__init__() got an unexpected keyword argument 'argument'

I think the second approach is much more verbose and easier to debug than the first one, where the error is coming from the parent class without further details on which is the offending argument.

from simpeg.

In general, it is tough to remove **kwargs from __init__ scripts everywhere in SimPEG because we make use of multiple inheritance in many class. (See IP simulations, EquivalentSource simulations, EM simulations etc.).

To me it does make sense that the classes we intend users to work with (anything that is not a Base Class) should have them be explicit though.

from simpeg.

There are a number of places where I have noticed people getting tripped up with **kwargs (most recently in some of the receiver classes). I am supportive of taking a first step and removing from public classes (and doing so in stages).

from simpeg.

@santisoler Just be aware that you need to keep in mind that we don't just use linear inheritance as in your example. Many of the simulation classes have diamond inheritance i.e. consider this typical structure:

class BasePDE():
    def __init__(self, mesh, **kwargs):
        print('BasePDE.__init__')
        self.mesh = mesh
        # Maybe some class has a mixin that needs to be initalized, so still would need to call super...
        super().__init__(**kwargs)

class BaseElectricalPDE(BasePDE):
    def __init__(self, conductivity, **kwargs):
        print('BaseElectricalPDE.__init__')
        super().__init__(**kwargs)
        self.conductivity = conductivity

class BaseMagneticPDE(BasePDE):
    def __init__(self, permeability, **kwargs):
        print('BaseMagneticPDE.__init__')
        super().__init__(**kwargs)
        self.permeability = permeability

class ElectromagneticPDE(BaseElectricalPDE, BaseMagneticPDE):
    def __init__(self, mesh, conductivity, permeability):
        print('ElectromagneticPDE.__init__')
        super().__init__(mesh=mesh, conductivity=conductivity, permeability=permeability)

The challenge would be to do this without using **kwargs, in way that only causes BasePDE.__init__ to be called once, while making sure that BaseMagnetic and BaseElectrical still initialize their parent when used individually.

from simpeg.

I also feel it's then cumbersome as a developer to add a feature to a base class, then have to go find every class that inherits from it and modify their call signatures.

from simpeg.

For each base class that has object as a parent, we could add a call that checks if the call to super() will call object, and return a better error message.

def _check_base_super(self, this_class, **kwargs):
    mro = type(self).__mro__
    super_class = mro[mro.index(this_class) + 1]
    if super_class is object:
        if len(kwargs):
            for key in kwargs:
                raise TypeError(f"{type(self).__name__} got an unexpected keyword argument '{key}'.")

which could be used as:

class BasePDE():
    def __init__(self, mesh, **kwargs):
        _check_base_super(self, __class__, **kwargs)
        super().__init__(**kwargs)
        print('BasePDE.__init__')
        self.mesh = mesh

from simpeg.

For each base class that has object as a parent, we could add a call that checks if the call to super() will call object, and return a better error message.

def _check_base_super(self, this_class, **kwargs):
    mro = type(self).__mro__
    super_class = mro[mro.index(this_class) + 1]
    if super_class is object:
        if len(kwargs):
            for key in kwargs:
                raise TypeError(f"{type(self).__name__} got an unexpected keyword argument '{key}'.")

which could be used as:

class BasePDE():
    def __init__(self, mesh, **kwargs):
        _check_base_super(self, __class__, **kwargs)
        super().__init__(**kwargs)
        print('BasePDE.__init__')
        self.mesh = mesh

In fact this is something we should probably do now to save a bit of headache for unsupported keyword arguments until we resolve all of this discussion.

from simpeg.

@santisoler Just be aware that you need to keep in mind that we don't just use linear inheritance as in your example. Many of the simulation classes have diamond inheritance i.e. consider this typical structure:

class BasePDE():
    def __init__(self, mesh, **kwargs):
        self.mesh = mesh
        # Maybe we use the BasePDE as a mixin for some other class, so still would need to call super...
        super().__init__(**kwargs)

class BaseElectricalPDE(BasePDE):
    def __init__(self, conductivity, **kwargs):
        super().__init__(**kwargs)
        self.conductivity = conductivity

class BaseMagneticPDE(BasePDE):
    def __init__(self, permeability, **kwargs):
        super().__init__(**kwargs)
        self.permeability = permeability

class ElectromagneticPDE(BaseElectricalPDE, BaseMagneticPDE):
    def __init__(self, mesh, conductivity, permeability):
        super().__init__(mesh=mesh, conductivity=conductivity, permeability=permeability)

The challenge would be to do this without using **kwargs, in way that only causes BasePDE.__init__ to be called once, while making sure that BaseMagnetic and BaseElectrical still initialize their parent when used individually.

I agree that **kwargs is one of the best options for solving diamond inheritance. I'm not against kwargs in general, and I totally support them if there's a very good reason for them.

I'm just raising the concern that even in these simple lines there's a level of complexity that most people using Python aren't too familiar with (mro, multiple inheritance, mixins, etc). My goal when raising this discussion it to reduce the level of complexity throughout the codebase where possible. But obviously, some implementations are best solved with more complex code, and that's fine.

from simpeg.

For each base class that has object as a parent, we could add a call that checks if the call to super() will call object, and return a better error message.

def _check_base_super(self, this_class, **kwargs):
    mro = type(self).__mro__
    super_class = mro[mro.index(this_class) + 1]
    if super_class is object:
        if len(kwargs):
            for key in kwargs:
                raise TypeError(f"{type(sim).__name__} got an unexpected keyword argument '{key}'.")

which could be used as:

class BasePDE():
    def __init__(self, mesh, **kwargs):
        _check_base_super(self, __class__, **kwargs)
        super().__init__(**kwargs)
        print('BasePDE.__init__')
        self.mesh = mesh

I don't have strong feelings regarding this check, but from a broader point of view I see it's trying to solve an interface problem in a base class. I'm usually in favor of separating interface from abstractions, mainly because in the long run it provides more flexibility when refactoring underlying code, and it allows to raise simpler error messages that are easier to understand.

from simpeg.