allow metaclass keywords in Model subclasses - #11017

Hello and thanks! Can you please clarify what use cases that this patch is trying to solve? Also, this is technically an API change, so it needs a change log and perhaps some user documentation update as well. And tests.

@pllim, sure thing. The general use case is allowing modifications to the Model class construction process in the metaclass, rather than in new, which is not the best place for it. For instance, the Model subclasses have the inverse property overwritten by the metaclass, changing it to _inverse and using the Model.inverse function. While this is fine for Model and most subclasses, there is no good way to control this action. Subclasses of Model which wish to exert such type of control need to be able to accept an argument to do so. This may be implemented as a class attribute, or, for a more inviolable option, as an arg/kwarg to the class construction itself.

class ExampleModel(Model, metaclass=_ExtendedModelMeta, some_flag=True):
    pass

This PR just makes this possible to implement _ExtendedModelMeta by users foolhardy enough to wade into the murk of metaclasses.

From my experience, I want to override the signature of the evaluate method for a series of subclasses of Polynomial models. Aspects of this process are best / most efficiently accomplished at class declaration, not instantiation. Having flags allows the classes to be powerfully tailored in a manner which may be changed in the child classes and store the choices with inheritable private attributes.

@pllim squashed with tests, change-log, and docs.

Thanks. Now let's wait for review from modeling maintainers. In the meantime, we ask for your continued patience.

Looks good

codecov seems to be getting this wrong, none of the coverage changes are related to this PR.

See comment

Great. I can rebase as necessary. Or vice versa if this is approved first.

I'm still a little confused about this. What is the specific case motivating this? I'm not aware of any case where it is necessary to subclass _ModelMeta, nor do I think the user documentation should say anything about extending _ModelMeta.

It does seem like accepting some arguments to Model subclasses could be useful in general though.

You mentioned something about the default _ModelMeta wrapping inverse. Is there some reason you need it to not do that? What is that reason? And if so, maybe there is a more directed solution to that problem that can be resolved internally, rather than exposing such machinery at a user-documented level.

@embray The specific case that motivated this is something in my personal research.

I want to override the signature of the evaluate method for a series of subclasses of Polynomial models. Aspects of this process are best / most efficiently accomplished at class declaration, not instantiation. However, when there is a custom metaclass, this type of manipulation is only possible if the metaclass allows for kwargs. _ModelMeta was implemented without kwargs, which it didn't need. I just want to add **kwargs to the metaclass. These are non-functional on _ModelMeta, but allow custom subclasses a greater degree of flexibility.

All that happens with this PR is that

class MyModel(Model, flag=True):
    ...

doesn't error. flag isn't used by _ModelMeta, but it could be by _MyModelMeta(_ModelMeta) in the following example

class MyModel(Model, metaclass=_MyModelMeta, flag=True):
    ...

It does seem like accepting some arguments to Model subclasses could be useful in general though

agreed!

@nstarman What do you mean by "override the signature of the evaluate method for a series of subclasses of Polynomial models" and is there a reason this couldn't be done with a simple class attribute?

@nden @embray @perrygreenfield, the above discussion, while relevant to the reason I pushed this PR, I feel is actually unrelated to the content of this PR, which only enables extending the Model metaclass, and doesn't actually do so here. This PR is certainly not critical for v4.3 and could be pushed to v5.0, but... #ready-for-final-review ?

I had a look at that GSL_only example, and while I see what that's doing what confuses me is still why this syntax of passing arguments in the class statement needs to be used at all in this case, as opposed to the more common syntax of just declaring this in a class attribute like:

class SCFPotential(CPotentialBase):
    GSL_only = True
    ...

There's certainly nothing wrong with the other approach. It's just less common and seems like overkill to me. But maybe this is purely an aesthetic/subjective question. It got me thinking, and I really don't know what a good use case is for custom arguments to class so I asked about this here: https://discuss.python.org/t/motivation-for-custom-keyword-arguments-to-class-statement/8402

But maybe there's something I'm completely missing as to why this is necessary. So I'm definitely not opposed to this PR, and and mostly just struggling to understand the use case.

But maybe there's something I'm completely missing as to why this is necessary. So I'm definitely not opposed to this PR, and and mostly just struggling to understand the use case.

It's funny because I worked around my use case as well :p ... As I understand it, the reasons for class arguments are twofold:

1. An argument to the class itself need never be stored. It can set a behavior and then be done. flags on the class should be kept (for clarity) by the metaclass and can be mutated like any other class method.
2. clarity: arguments on the class can be anything, e.g. n_inputs, while arguments to the class are very explicitly arguments to the metaclass constructor.

In the context of https://discuss.python.org/t/motivation-for-custom-keyword-arguments-to-class-statement/8402, that example is just a flag. But one idea for something that could be done that is more complicated is: wrapping methods for numpy ufuncs versus numba. More complicated things could be done, especially since this example was just using __init_subclass__ and not directly touching a metaclass.

class ScalarClassWithMethods:
    def __init_subclass__(cls, wrap_method):
        wrapper = numba_wrapper if wrap_method == "numba" else numpy_wrapper
        for name, method in _get_my_methods_:
            setattr(cls, name, wrapper(method))

class NumpyWrapped(ScalarClassWithMethods, wrap_method="numpy"):
    pass

class NumbaWrapped(ScalarClassWithMethods, wrap_method="numba"):
    pass

Seems like discussions are continuing... I am going to re-milestone to 5.0 so we don't make rash decisions due to lack of time.

I am re-milestoning to v5.1 as we've now branched v5.0.x

Hi humans 👋 - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock.

If you believe I commented on this pull request incorrectly, please report this here.

I'm going to close this pull request as per my previous message. If you think what is being added/fixed here is still important, please remember to open an issue to keep track of it. Thanks!

If this is the first time I am commenting on this issue, or if you believe I closed this issue incorrectly, please report this here.

Hmm, this was approved by @nden back in Dec 2020 but has been languishing since and yet has a "ready for final review" label. Should the label be removed? What is the current status? Thanks!

Personally, I think it's ready to merge. I might actually remove some of the tests. I'm not sure if the change is large enough to merit testing.

Does @nden want to re-review or is an approval from @WilliamJamieson enough?

William is a maintainer.
Time to merge this.