Repository navigation
Model.inverse tweaks - #4236
Model.inverse tweaks#4236
Conversation
…now, I think, simpler. Rather than having to generate an inverse property for every single model class that defines an inverse, it pops the inverse property off a model subclass and stores it off in the model._inverse internal attribute. Then it deletes the inverse property from the subclass so that model.inverse is resolved to the base class's Model.inverse which knows to use the inverse method from ._inverse if it's defined.
…verse, which makes a little more sense than model.inverse = None (which seems to imply explicitly setting a model to have no inverse). However, the current behavior is still supported for backwards-compat (but deprecated).
…unction to evaluate it (this is demonstrated into the intro too, but it should also be made clear in this section). Also deleted a section that was almost entirely outdated information.
…o added an inverse for the Linear1D model, which I only realized didn't exist until writing this documentation example (which was silly because the example linear model in the documentation does define an inverse, so the actual built-in model ought to have it too...
|
Note: Most of this change is just an internal change to the implementation and not user-visible. But the addition of |
|
@nden As a heavy user of inverses what do you make of these changes? In short, the current behavior is still kept. But in the future setting |
|
I should add though, the behavior I've implemented here for resetting an inverse to its default is consistent for the Pythonic behavior of classes with a default class attribute that is overridden on an instance: >>> class MyObj(object):
... foo = 'a'
...
>>> my_obj = MyObj()
>>> my_obj.foo
'a'
>>> my_obj.foo = 'b'
>>> my_obj.foo
'b'
>>> del my_obj.foo
>>> my_obj.foo
'a'In the case of the inverse, a user may set (In the case of model.inverse it's a little more complicated implementation-wise since inverse is still an instance-dependent property. But that's just an implementation detail--the important point is that the behavior implemented here is the same as the behavior for more trivial class attributes.) |
|
@embray I'm OK with this in principle but my code currently crashes with this PR. I need to track down what causes it. |
|
How does it crash? If there's a traceback or anything I could maybe glean something from it. This should be backwards compatible with the exception of raising a warning when assigning None. That case is covered by the tests too, but there could be some corner case? |
|
I tracked the crash to pyasdf. It doesn't roundtrip when a model has a custom inverse. |
|
IIRC pyasdf might have been digging around in some internal variables that I changed. I'll have a look... |
|
Yeah, it's as I thought. I just renamed I could see it being a performance tweak. Or maybe just a way to check if there's a custom inverse (since otherwise it wouldn't have to store it I guess). There should be a better way to indicate whether or not the inverse set on a model is a hand-provided inverse or not. I think I remember you or Mike bringing that up at one point. I'll add something for that. |
|
I think to keep things simple for now I'll just undo the renaming of that internal attribute so that pyasdf keeps working with this for now. In the long term I'll look into a better solution. |
|
Actually, on second thought, I started writing such an issue and then realized there's a very trivial solution to this, which would be to just add a |
…backwards-compat for pyasdf.
|
@nden I updated this to keep working with the current code in pyasdf. Could you give it a try (or just post some test code I can try?) Thanks. |
|
@embray I ran a few tests and don't see any problems now. |
|
Thanks for checking! |
a bounding box on a model. The format of the bounding box itself (as a tuple of tuples) is not changed. This is motivated in large part by wanting the API to more closely resemble the API for inverses which is somewhat simiar--model.inverse is a property of a model that returns something based on a model instance *if* the inverse is defined on that model's class. Otherwise it raises NotImplementedError. Regardless of whether a default inverse is defined on the class, the user can assign their own inverse to any model instance. The user-defined inverse can unset (as of PR astropy#4236) by calling del model.inverse, which will reset it back to the inverse defined at the class level. This PR makes bounding_box work entirely analogously to the above. The bounding_box_default() method is removed--instead, in order to define a bounding box for a specific model class, simply define a property called bounding_box on the class (the metaclass will then handle) wrapping that property in the standard getter/setter for Model.bounding_box defined in the Model base class). The one extra complexity for bounding_box is that we want *some* cases of bounding_box to be *callable* with adjustable parameters (such as how Gaussian1D.bounding_box_default() takes an optional "factor" argument). To this end, the object returned by model.bounding_box is actually a subclass of tuple that looks like a normal bounding box tuple. But on a model whose bounding_box takes arguments (and is a method instead of a property) one can *call* model.bounding_box like ``model.bounding_box(factor=1.2)`` to adjust the bounding box.
a bounding box on a model. The format of the bounding box itself (as a tuple of tuples) is not changed. This is motivated in large part by wanting the API to more closely resemble the API for inverses which is somewhat simiar--model.inverse is a property of a model that returns something based on a model instance *if* the inverse is defined on that model's class. Otherwise it raises NotImplementedError. Regardless of whether a default inverse is defined on the class, the user can assign their own inverse to any model instance. The user-defined inverse can unset (as of PR astropy#4236) by calling del model.inverse, which will reset it back to the inverse defined at the class level. This PR makes bounding_box work entirely analogously to the above. The bounding_box_default() method is removed--instead, in order to define a bounding box for a specific model class, simply define a property called bounding_box on the class (the metaclass will then handle) wrapping that property in the standard getter/setter for Model.bounding_box defined in the Model base class). The one extra complexity for bounding_box is that we want *some* cases of bounding_box to be *callable* with adjustable parameters (such as how Gaussian1D.bounding_box_default() takes an optional "factor" argument). To this end, the object returned by model.bounding_box is actually a subclass of tuple that looks like a normal bounding box tuple. But on a model whose bounding_box takes arguments (and is a method instead of a property) one can *call* model.bounding_box like ``model.bounding_box(factor=1.2)`` to adjust the bounding box.
|
Backported to v1.1.x in 357c1fc |
a bounding box on a model. The format of the bounding box itself (as a tuple of tuples) is not changed. This is motivated in large part by wanting the API to more closely resemble the API for inverses which is somewhat simiar--model.inverse is a property of a model that returns something based on a model instance *if* the inverse is defined on that model's class. Otherwise it raises NotImplementedError. Regardless of whether a default inverse is defined on the class, the user can assign their own inverse to any model instance. The user-defined inverse can unset (as of PR astropy#4236) by calling del model.inverse, which will reset it back to the inverse defined at the class level. This PR makes bounding_box work entirely analogously to the above. The bounding_box_default() method is removed--instead, in order to define a bounding box for a specific model class, simply define a property called bounding_box on the class (the metaclass will then handle) wrapping that property in the standard getter/setter for Model.bounding_box defined in the Model base class). The one extra complexity for bounding_box is that we want *some* cases of bounding_box to be *callable* with adjustable parameters (such as how Gaussian1D.bounding_box_default() takes an optional "factor" argument). To this end, the object returned by model.bounding_box is actually a subclass of tuple that looks like a normal bounding box tuple. But on a model whose bounding_box takes arguments (and is a method instead of a property) one can *call* model.bounding_box like ``model.bounding_box(factor=1.2)`` to adjust the bounding box.
3 participants
These changes started in the context of finishing up #4040. My goal in #4040 is to just make a few slight interface tweaks so that the
.bounding_boxattribute works consistently with how the.inverseattribute works, since they have a few similarities (ex: Some models have a default method that produces a bounding box, but not all, and in either case users can override the default bounding box with a custom one, but can also reset back to the default if they wish).In working on that I realized there are some small tweaks I can make to how
.inverseis implemented that will also make.bounding_boxeasier to do in the same way, and that will also make it easier to create a consistent interface between them. In particular I want to be able to reset back to the default by using thedeloperator. This is new functionality, but I need it in order to finish up #4040 in a consistent manner.