Model refactoring stage 2#2835
Conversation
|
Relevant updates to the documentation are forthcoming. |
There was a problem hiding this comment.
This is some voodoo magic right here 😉 It might belong in astropy.utils proper at some point, but fine to be here for now.
There was a problem hiding this comment.
I suspect it could be found useful elsewhere, but I haven't looked closely.
|
Looks good. It certainly simplifies the writing of all of the individual concrete model classes. I look forward to seeing how this enables compound models... |
|
It's not that it's absolutely required for compound models. I already have them partially working without this. But it will help a lot with a lot of the coordinate foolery we've talked about. That, and making compound models work somewhat transparently like any other model. At least that's the hope. |
|
I definitely like this change a lot from a high-level point of view, but will try to do a more thorough review later if I can find the time. |
<astropy#2835 (comment)> this adds the make_func_with_sig function to astropy.utils.misc. Taking advantage of this, a new astropy.utils.compat.misc.wraps function is provided as a replacement for functools.wraps, which provides the same functionality but while also preserving the original wrapped function's signature (so that it displays properly in help())
<astropy#2835 (comment)> this adds the make_func_with_sig function to astropy.utils.misc. Taking advantage of this, a new astropy.utils.compat.misc.wraps function is provided as a replacement for functools.wraps, which provides the same functionality but while also preserving the original wrapped function's signature (so that it displays properly in help())
|
I still need to make some updates to the documentation as part of this PR, but at that point I'll go ahead and merge if there are no objections (better to get as many of these changes as possible in sooner rather than later). |
There was a problem hiding this comment.
Trvial comment, but why not just make_function_with_signature?
There was a problem hiding this comment.
That would be fine by me...
|
Changes seem very nice, but see my few small comments. |
…onger needs to include its own copy of the make_func_with_sig utility. I also renamed make_func_with_sig to make_function_with_signature as suggested here: astropy#2835 (comment) Previously I thought the unabbreviated name was a little on the long side. But on second thought I prefer the clarity here.
|
Rebased, and also addressed the comment at #2835 (comment) |
…onger needs to include its own copy of the make_func_with_sig utility. I also renamed make_func_with_sig to make_function_with_signature as suggested here: astropy#2835 (comment) Previously I thought the unabbreviated name was a little on the long side. But on second thought I prefer the clarity here.
…onger needs to include its own copy of the make_func_with_sig utility. I also renamed make_func_with_sig to make_function_with_signature as suggested here: astropy#2835 (comment) Previously I thought the unabbreviated name was a little on the long side. But on second thought I prefer the clarity here.
…onger needs to include its own copy of the make_func_with_sig utility. I also renamed make_func_with_sig to make_function_with_signature as suggested here: astropy#2835 (comment) Previously I thought the unabbreviated name was a little on the long side. But on second thought I prefer the clarity here.
…onger needs to include its own copy of the make_func_with_sig utility. I also renamed make_func_with_sig to make_function_with_signature as suggested here: astropy#2835 (comment) Previously I thought the unabbreviated name was a little on the long side. But on second thought I prefer the clarity here.
…onger needs to include its own copy of the make_func_with_sig utility. I also renamed make_func_with_sig to make_function_with_signature as suggested here: astropy#2835 (comment) Previously I thought the unabbreviated name was a little on the long side. But on second thought I prefer the clarity here.
…onger needs to include its own copy of the make_func_with_sig utility. I also renamed make_func_with_sig to make_function_with_signature as suggested here: astropy#2835 (comment) Previously I thought the unabbreviated name was a little on the long side. But on second thought I prefer the clarity here.
…onger needs to include its own copy of the make_func_with_sig utility. I also renamed make_func_with_sig to make_function_with_signature as suggested here: astropy#2835 (comment) Previously I thought the unabbreviated name was a little on the long side. But on second thought I prefer the clarity here.
…ather than just counts of inputs/outputs.
…gs the same code generation used by custom_model to the main Model class itself for __call__ methods. Now it's generally unnecessary for Model subclasses to redefine __call__ even if they want to update the method signature to reflect the names of their inputs. That is now automatic. Also the Model base class no longer makes any assumptions about the number of a model's inputs or outputs. It assumes no inputs/outputs until otherwise specified in a subclass.
…t came up, interestingly, when trying to call help() on a model class.
…h are no longer needed due to the automatic code generation in the _ModelMeta metaclass.
… generating __init__ methods as well. This allows still more boilerplate to be removed from most Model subclasses (though it still allows individual subclasses to define their own __init__ when needed--see for example the polynomial models).
…orator, since that part is now already handled (and somewhat better) by _ModelMeta.__new__
560d38b to
2c603f7
Compare
|
Rebased this PR. If nobody objects I'm going to go ahead and update the documentation to go along with the changes in this PR and then merge. So far I haven't been able to take as much advantage of the named inputs/outputs as I originally thought I might, but I think it's still a useful feature to have available (and certainly not particularly harmful). One addition I might make is to allow model classes that define |
|
On the other hand I just tested this PR out against the test suites for specutils, photutils, and ccdproc and didn't encounter any issues so maybe I don't need to bother with any additional backwards-compat changes. |
…or the Pix2Sky/Sky2Pix base classes. Unfortunately the only obvious way to add them is to include them in the module's __all__ even though they're not generally useful for users (though I guess useful as base classes for new projection types?) If there's another way to do this let me know. This is okay in the meantime though.
|
The latest update should fix the problem with the docs in the build. I still need to update the docs themselves a bit more--specifically the part on how to implement new models. Other than that this is ready for final review. |
… of 0.0 in the __init__, but this default is not given to the Parameter descriptor itself.
…o longer generally useful to manually define an __init__ and __call__ (though an example of where it is useful to define a custom __init__ is given) and the roles of the input/output class attributes are discussed.
…s that could use more details, and fixed __module__ and __qualname__ on generated __init__ and __call__ methods on models.
…onger needs to include its own copy of the make_func_with_sig utility. I also renamed make_func_with_sig to make_function_with_signature as suggested here: astropy#2835 (comment) Previously I thought the unabbreviated name was a little on the long side. But on second thought I prefer the clarity here.
|
These changes look good to me - I looked through the code, and while I didn't review every single line carefully enough to guarantee there are no issues, in general I'm in favor of these changes. Even if astropy.modeling is not API-stable, it might still be worth mentioning the changes in the changelog (for our sake) |
|
Ack, I thought I did put it in the changelog. Will definitely do that before merging. I also will write a summary of this and other coming changes for the what's new page. |
|
Sounds good, you can always add |
…the 'whats new' page for 1.0 [ci skip]
This is the followup to #2580, though it also builds upon (the as of yet unmerged) implementation for #1763.
The major changes here, thus far, are:
n_inputsandn_outputsinteger attributes, model classes now have class attributes called justinputsandoutputs, each of which is a tuple giving unique names to each input coordinate and output that a model has.n_inputsandn_outputsare still support, but simply return the lengths of these tuples.One thing that might still take some thinking is some sensible names for the inputs out and outputs of each model, but so far several defaults are used that are consistent with the existing implementations.
For subclasses of
Model, the function signature for its__call__method automatically reflects the names of the model'sinputs(unless__call__has been manually defined by the model's author, though there's usually no reason to do this since the changes in Model refactoring stage 1: "evaluate" normalization #2580).Likewise, the signature for each model's
__init__method automatically reflects the model's parameter names, unless the class's author has manually provided an__init__method. This is a little more common (see the polynomial models) than overriding__call__. But for most models this still allows cutting out a lot of boilerplate.If this seems like it's starting to go overboard with the metaprogramming, keep in mind that a lot of this work is in support of defining compound models. Some of this is still a bit experimental though and could use some feedback (I have already tested gammapy and photutils successfully against this PR).