bounding_box refactor

[WilliamJamieson]

Description

The underlying representation of bounding boxes in ~astropy.modeling is highly problematic at times. This is because currently multi-input bounding boxes are essentially tuples of tuples ordered in the "python/C" ordering (i.e. if the inputs are x, y, z then the bounding box is ordered z, y, x, see PR #11908 for more details). Moreover single dimensional inputs are represented as a single tuple.

This becomes an issue because inside ~astropy.modeling the natural x, y, z ordering is generally used, meaning that the bounding box tuple needs to be reversed in most cases (and then often reversed again to keep the representation constant). Moreover, in libraries like gwcs all the bounding boxes are represented in the natural ordering with constant flips of the box into and out of modeling because the natural representation is easier for humans to understand as it fits normal mathematical convention. Thus the user should be able decide what ordering they want to use.

The BoundingBox class (replacing the old _BoundingBox class) accomplishes this by storing each input's bounding interval in a dictionary keyed of the input's index. It allows access to the component intervals via either the input's string name (e.g. 'x') or its input index (e.g. 0), which in any case allows this BoundingBox to be more agnostic to this ordering.

This new BoundingBox accepts the same construction inputs as the current bounding box (tuples of tuples), can be compared directly with the old representation, and provides a method for directly outputting this representation. It also provides an order keyword that allows one to specify the tuple ordering meaning that users can now decide which ordering they which to use instead of requiring constant order reversals (if they choose the natural ordering).

As part of this refactor, I have also moved the methods in ~astropy.modeling.core which deal with bounding box enforcement into the BoundingBox class itself. This simplifies some of the code in ~astropy.modeling.core, while moving it to a more natural location for better maintainability.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

• Do the proposed changes actually accomplish desired goals?
• Do the proposed changes follow the Astropy coding guidelines?
• Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
• Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
• Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see "When to rebase and squash commits".
• Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the Extra CI label.
• Is a change log needed? If yes, did the change log check pass? If no, add the no-changelog-entry-needed label.
• Is a milestone set? Milestone must be set but astropy-bot check might be missing; do not let the green checkmark fool you.
• At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate backport-X.Y.x label(s) before merge.

[github-actions]

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

[perrygreenfield]

A comment on the above description. For astronomers, who think in x, y ordering (setting aside z) for image corresponding to the index conventions they are used to, namely x, y, not y, x. For them Python is representing their coordinates in y, x order since the array storage has the last index varying most rapidly. This is contrary to the FITS sense of index order, where x is the first index and it varies most rapidly.

All this, of course, is. a source of great confusion as you have different communities that think the other is backwards (and not natural). So I would avoid the term natural, unless you are referring to the fact that Python represents arrays most naturally in y, x order if x is considered to be the most rapidly varying coordinate.

[perrygreenfield]

One question: even though I've seen a number of model files changed to support this, should I presume that if others have written models already, that they are not forced to modify them to work with bounding boxes.

Also, I must admit I ran out of energy and did not review test_bounding_box.py

[perrygreenfield]

Is this description correct? I.e., "constructs" instead of "validates"?

[WilliamJamieson]

validate can be used to construct a valid bounding box from the old representation.

[WilliamJamieson]

See 9ca6b36.

[perrygreenfield]

This sort of thing has two purposes, validation and massaging the input into a standard representation. It almost seems like it should have its own name or terminology since it isn't simply validation. Validate and regularize or something like that. This is more of a pointless comment that is more for future terminology than in this case.

[pllim]

I don't know how I feel about this. This is going to break my downstream packages big time, won't it?

https://github.com/spacetelescope/synphot_refactor/blob/3cc67bc7142f3c3b399c8d5c78cda06a775156fa/synphot/models.py#L64

[WilliamJamieson]

This new bounding box will function just fine with descriptions so long as one respects the bounding_box interface in modeling. That is either using the bounding_box setter or defining a bounding_box method (as your link references, note that there are astropy models like Gaussian1D and Gaussian2D which function in the same way). All of the necessary changes have been made to Model so the previously established methods of defining bounding boxes still work by performing a conversion to the "new" bounding box if necessary.

As far as the change to the documentation referenced here, I simply added a more descriptive __repr__ to the new bounding box so that users could more easily determine how their bounding box was being interpreted (its clear what is x, y and z input intervals).

[pllim]

Okay... I guess I'll just have to see how many things break after this is merged, but at least I know who to bother... 😸

[WilliamJamieson]

I have a set of changes for gwcs in place to accept this bounding box once it gets merged (mostly to remove the need for constant flipping of the bounding box order) and one tiny change to jwst (a warning filter needs to be added to a single test).

[pllim]

Can you please xref those PRs here for future reference? Thanks!

[pllim]

@perrygreenfield or @nden , will this need a What's New entry?

[WilliamJamieson]

The failing CI test will be fixed by PR #11972.

[nden]

@pllim I am going to merge this. Please let us know of unintended problems in your packages.

[pllim]

unintended problems in your packages

At a glance, things will probably break but doesn't look too hard to fix, unlike the blackbody stuff way back when.