Update bounding_box property and new Model.render() method in astropy.modeling - #4040

@adonath, @cdeil - I rebased against master so only updates since #3909 are in the diff.

It unrelated to the changes in this PR, but I just noticed, that https://github.com/astropy/astropy/pull/4040/files#diff-78d4f451e670d4d6fee33e0c0053a673L32 doesn't render properly to a link.
Probably it not possible to link to class methods or attributes... Any ideas @cdeil?

Again not related, but could fix the pep8 formatting here? https://github.com/astropy/astropy/pull/4040/files#diff-78d4f451e670d4d6fee33e0c0053a673R39

@adonath I'd go even further and ask what is the purpose of adding a wcs parameter to render_model? We've managed to keep modeling pretty flexible and general so far. I can see how evaluating a model over certain range of validity is useful while still keeping the code general. Adding a parameter which is a specific implementation of a specific WCS standard suddenly decreases the value of the package in my view. Isn't this accomplished by a composition model, something like WCS.wcs_world2pix | model where the model is evaluated over the bounding box (Assuming the first one is wrapped as a modeling.Model)?
Or am I missing something (quite possible)?
This is still valuable but render_model looks like a very good utility function somewhere else, not in modeling. Perhaps @embray has some thoughts on this?

@adonath - regarding the bounding_box_default link - it appears to work fine for me locally.

The link doesn't work on readthedocs:
http://astropy.readthedocs.org/en/latest/modeling/bounding-boxes.html

@nden I see what you mean. The reason to add a WCS argument to 'render_model', was mainly
a convenience option for the user and should allow to define models in world coordinates. So internally
this is just used to convert the pixel coordinate arrays to world coordinate arrays, where the model is evaluated on. Otherwise this would have to be done outside by the user (which is in principle ok). So it's kind of the opposite, what you suggested with WCS.wcs_world2pix | model, where the model is defined in pixel coordinates. The problem is that when the model (and bounding box) are defined in world coordinates, the evaluation function has to know about the WCS object, to compute the correct pixel positions for the bounding box region, to add it back in the larger data array.

@adonath OK. This just looks too limited to me and not something that belongs in modeling.core. Perhaps a convenience function in wcs.utils (shouldn't distoriton be included as well)?

I wonder if bounding_box should be allowed to be a model with the requirement that when evaluated it should return the bounding box in the desired frame. I'd be OK with adding something like that to the base class Model. GWCS takes this approach for a similar purpose, e.g. one of the LabelMapper classes in spacetelescope/gwcs#18 .

Perhaps @perrygreenfield @embray @mdboom have some thoughts on this.

I agree with @nden that the wcs parameter as such should be removed, as it's too particular to FITS WCS as-is. We'll work something else out for that in the meantime.

The code for it still looks useful though and is worth saving. Maybe instead it could be a method or property on the WCS class? (WCS-to-bounding-box?) Or a utility function that lives somewhere else (I'm not sure where though).

@embray @nden (also @adonath and @cdeil)

I made several changes today but haven't yet updated all docs and tests so I'm not worried about the builds failing at this point.

I removed the wcs parameter in this PR, but I saved the wcs implementation in a separate branch and will open an issue linking to it in case anyone wants to pick it up later. I won't have the time myself in the near future.

@embray - I think all of your comments on #3909 have now been addressed in this PR, except for reworking bounding_box into a class since that's a much greater task. I'm not entirely sure about the way I initialize bounding_box_default since I sort of just hacked my way to a solution. It seems to be working though it's not fully tested.

The model.render() method currently just calls the render_model function. It seems redundant to have both the method and standalone function, so the question is whether it should be a private function, probably in modeling.utils, or if I should do away with it entirely and copy the code into the method. Thoughts?

@adonath, @embray - ignore this for now, I might be coming around on some points as I work through tests but I'm about to get on a plane so I'll get back to you later.

I think we want to set _bounding_box to its appropriate default value on initialization when a default function is implemented. If this isn't done in the Model class, then _bounding_box also needs to be explicitly overridden in the subclass. By defining it in the Model class, you can just forget about _bounding_box in the subclasses. As a user, if I define a custom model with a bounding_box_default method, I should be able to expect new_model.bounding_box to immediately return the default limits without first setting new_model.bounding_box = 'auto'. Like in the doc example, this line shouldn't be necessary. The way I have it written now, this happens automatically.

Secondary to that, I want to raise the NotImplementedError in the bounding_box property and have bounding_box_default return None because I want to set _bounding_box = bounding_box_default(). This isn't possible if it raises an error. Further, passing None to the setter "turns off" the bounding box, so the case that _bounding_box == None is already handled (with a NotImplementedError).

At the end of the day, I'll do whatever you think is best, but just wanted to make my case.

I would just set the default value _bounding_box = 'auto' at the class level. Then:

@classmethod
def bounding_box_default(cls):
    raise NotImplementedError('blah blah blah')

@property
def bounding_box(self):
    not_implemented = False
    if self._bounding_box is None:
        not_implemented = True
    elif self._bounding_box == 'auto':
        try:
            # In retrospect, this should be recomputed every time since the bb *may*
            # change if any of the parameters are updated
            return self.bounding_box_default()
        except NotImplementedError:
            not_implemented = True

    if not_implemented:
        raise NotImplementedError("blah blah blah")

    return self._bounding_box

@bounding_box.setter
def bounding_box(self, limits):
    if isinstance(limits, six.string_types) and limits.lower() == 'auto':
        try:
            self.bounding_box_defaults()
        except NotImplementedError:
            raise NotImplementedError("Can't set bounding_box blah blah blah")
        limits = 'auto'
    elif ...:
        ...

    self._bounding_box = limits

I think most of the above should satisfy your requirements.

This is a little different than before, since previously I think I suggested avoiding storing the actual value 'auto' in the ._bounding_box attribute. But now it occurs to me that auto bounding-boxes should always be recomputed in case any parameter values have been changed (unless there is some way to flag whether or not a model's bounding_box_defaults depends on the parameters, but I think it would in most cases...).

If that seems unsavory than a separate attribute could be used to flag whether auto-bb is enabled.

I would avoid any solution that requires initializing self._bounding_box in Model.__init__ (not all models will use this feature, and it's just as easily set later).

@embray - thanks, that's nearly exactly where I landed - I'm missing the exception in the setter, but that makes sense to have.

Great--glad we wound up on the same page.

@patti is this ready for reviewing? If so, could you rebase? Thanks!

I think @patti may be busy with school (?) which is fine if so. I'll need to finish this up if so, but getting this in for 1.1.0 is important.

@embray - can we delay this to 1.2 if it's not going to be possible to merge today? Is there a reason it has to be included in 1.1?

If I don't then I'll have to unmerge #3909. This PR makes changes to the API and I don't want to have to support the API that's in there...

In any case, as far as the "feature freeze" is concerned, the "feature" is already done in #3909. But it needs some more work before the final release.

@embray - I agree that as an exceptional case this could be merged after feature freeze since it's not adding a feature. But how soon do you think you could finish this up?

Okay, I'm going to try to get this wrapped up. I'm also thinking of making a slight interface change, so that instead of defining a bounding_box_default one can just define a bounding_box method on any model, and that defines the default implementation for that model (however it would still be possible to replace with a hard-coded one, as well as reset to the default). In other words, it would make the implementation analogous to how inverse works.

None of the rest will change--this is just an implementation tweak more than anything.

@embray Thanks for taking this over! The change of the interface sounds reasonable to me. Unfortunately I don't have time to work on it myself, but I might find some time this week to review the code again, if necessary.

@embray - what do you think about this: should we push it to 1.2 (or 1.1.1?)

I'm trying to finish this today. It's mostly done, I just need to finish some doc updates.

Hi all,

Sorry for the silence. I was on vacation off the grid and this thread got lost in the deluge of emails I came back to. Thanks for picking it up @embray - is there anything I can do to help at this point?

Patti

@patti thanks for checking in and no problem. I'm about to put up a PR for my followup to this with my slight changes to the interface. It'd be great if you can take a look and let me know what you think (though I'm going to try to get it merged very soon...)

Merged in #4264