bounding_box refactor#11930
Conversation
|
👋 Thank you for your draft pull request! Do you know that you can use |
|
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
left a comment
There was a problem hiding this comment.
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
|
|
||
| Methods | ||
| ------- | ||
| validate : |
There was a problem hiding this comment.
Is this description correct? I.e., "constructs" instead of "validates"?
There was a problem hiding this comment.
validate can be used to construct a valid bounding box from the old representation.
There was a problem hiding this comment.
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.
| >>> model = Ellipsoid3D() | ||
| >>> model.bounding_box | ||
| ((-4.0, 4.0), (-3.0, 3.0), (-2.0, 2.0)) | ||
| BoundingBox( |
There was a problem hiding this comment.
I don't know how I feel about this. This is going to break my downstream packages big time, won't it?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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... 😸
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Can you please xref those PRs here for future reference? Thanks!
|
@perrygreenfield or @nden , will this need a What's New entry? |
24b6968 to
68c5f4b
Compare
|
The failing CI test will be fixed by PR #11972. |
68c5f4b to
ae0d43f
Compare
ae0d43f to
d4f96c2
Compare
cc57775 to
b8c6779
Compare
Most changes to integrate new bounding box Fixed broken tests Updated bounding_box and its unit tests to include equality Added pseudo support for model set bounding boxes Updated bounding_box to be better able to accept auto-generated bounding_box Integrated new bounding_box into automatic bounding_boxes Fixed final broken test. Removed an accidental print. Fixed asdf bug. Refactored order handling for bounding_box Removed references to the old _BoundingBox. Finished TODO to remove check on bounding box input processing. Moved more i/o handling for bounding box into bounding box class Switched RuntimeError to RuntimeWarning for interval bounds validation. Fixed warning to be warning not exception Added documentation Updated _fix_input_bounding_box to be in line with PR astropy#11908. Made fix_inputs_with_bounding_box more robust Fixed doctest failure Improved test_custom_model_bounding_box test to test with_bounding_box=True Removed debugging print from a test. Added changelog entry. Fixed unknown error in asdf round-trip for bounding_box Added `__all__` to bounding_box.py Moved changelog to correct folder. Addressed perrygreenfield's comments. Removed all references to `nan_index`. Changed `_model` to `model` in BoundingBox Added bounding_box to API reference. Updated docstrings for read-the-docs. Fixed more docstrings More docstring fixes New attempt at fixing docstring
b8c6779 to
7717ae7
Compare
|
@pllim I am going to merge this. Please let us know of 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. |
Description
The underlying representation of bounding boxes in
~astropy.modelingis 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.modelingthe 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 likegwcsall 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
BoundingBoxclass (replacing the old_BoundingBoxclass) 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 thisBoundingBoxto be more agnostic to this ordering.This new
BoundingBoxaccepts 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 anorderkeyword 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.corewhich deal with bounding box enforcement into theBoundingBoxclass 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.
Extra CIlabel.no-changelog-entry-neededlabel.astropy-botcheck might be missing; do not let the green checkmark fool you.backport-X.Y.xlabel(s) before merge.