Adding `CompoundBoundingBox` #11942

WilliamJamieson · 2021-07-13T20:37:59Z

Description

Often one ends of with models for images with many slices have a dependency on that slice. These models can be extremely complex, being comprised of ~100 models, which are nearly identical for all slices, but have different bounding boxes for each slice. The bounding box calculations for each slice can become expensive if there is a need to run the model multiple times. For this reason, it is useful to be able to bind a "compound" bounding box to the model, which can select the "correct" bounding box based on a slice argument.

In this pull request, I introduce a CompoundBoundingBox which essentially is a dictionary of bounding boxes with keys given by the slice values. When the bounding_box is used to evaluate the model, the CompoundBoundingBox first selects the correct bounding_box and then uses that bounding_box to evaluate the model.

This PR depends on PR #11930 and PR #11931 so a rebase should be performed after these PRs have been merged.

Note that this PR closes the draft PR #11699 as it accomplishes the same end goal.

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.

pep8speaks · 2021-07-19T18:36:13Z

Hello @WilliamJamieson 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style.

There are no PEP8 style issues with this pull request - thanks! 🎉

Comment last updated at 2021-10-30 00:07:20 UTC

WilliamJamieson · 2021-10-26T17:00:02Z

The failing tests seem to be unrelated to this PR. This seems to be the same problem as documented by issue #12298.

perrygreenfield

This is a general comment about my lack of a good sense of how this all ties together, which I can't easy grok from the source code or the test cases. (It is said that the test cases are a good place to see how a class or module should be used, but I don't think that is true here necessarily). So this is a bit of a conundrum. I see this as mostly a documentation issue, and as such, such things can be deferred after the freeze (as I understand it). On the other hand, the lack of this really prevents me from doing as complete review as I would like. Perhaps Nadia is in a better position to review the usage case since she has applied it to a JWST case. I'll try taking a look at her implementation, but I figured I would get these comments in first.

In looking at the examples in test_core.py I find the examples pretty artificial and still wonder about how it is to be used. For example, suppose I define a model that has extra inputs that specify a spectral order, or slit id, say model(x, y, slit_id). Can I call that as follows: model(100, 200, 23, with_bounding_box=True) and have the bounding box associated with 23 automatically used, or must I assign 23 to the bounding box keyword? (e.g., model(100, 200, 23, with_bounding_box=23). If not current, does the current design preclude that in the future? That is the interface I would like to have (that is, the compound bounding box is told what input values are used to select the correct one, and when it is used by the model, it is given that information automatically to select the appropriate case).

Also is it eventually possible to automatically associate the model to the bounding box when assigning it to the model's bounding box attribute. Currently it seems redundant, but I haven't studied it enough to see if it avoidable in the future. At the moment it seems unnecessarily circular. That is, define a model, then define a bounding box referring to that model, then assign the bounding box to the model.

Again, if the current design doesn't preclude these future interfaces, then I don't see it as a problem for now. If one wants to see what the bounding box is for specific inputs, then give them to the method to retrieve them.

astropy/modeling/bounding_box.py

astropy/io/misc/asdf/tags/transform/basic.py

WilliamJamieson · 2021-10-29T18:53:08Z

I think the current CI failures in this PR are from #12324, not this PR itself.

nden · 2021-10-29T21:27:10Z

This is ready to be merged after the unrelated CI failures are resolved.

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 Most changes to integrate new bounding box Integrated new bounding_box into automatic bounding_boxes Finished TODO to remove check on bounding box input processing. Updated _fix_input_bounding_box to be in line with PR astropy#11908. Made fix_inputs_with_bounding_box more robust Improved test_custom_model_bounding_box test to test with_bounding_box=True Removed debugging print from a test. Added changelog entry. Added `__all__` to bounding_box.py Fixed more docstrings Integrated new bounding_box into automatic bounding_boxes Finished TODO to remove check on bounding box input processing. Improved test_custom_model_bounding_box test to test with_bounding_box=True Removed debugging print from a test. Added `__all__` to bounding_box.py Fixed more docstrings Refactored generic_call Removed generic_call from core.py Refactored _generic_evaluate into Model class Moved get_bounding_box into model class Moved _prepare_inputs_* functions into model class Refactored _prepare_outputs_* into model. Refactored validate_input_shapes into model Refactored some helper functions into model. Fixed codestyle errors. Added changelog entry. Minor refactoring and additional documentation. More documentation added. Fixed get_bounding_box in asdf Moved changelog to correct folder Fixed rebase issue Refactored and added tests to increase test coverage. Added initial new bounding box 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 Most changes to integrate new bounding box Integrated new bounding_box into automatic bounding_boxes Finished TODO to remove check on bounding box input processing. Updated _fix_input_bounding_box to be in line with PR astropy#11908. Made fix_inputs_with_bounding_box more robust Improved test_custom_model_bounding_box test to test with_bounding_box=True Removed debugging print from a test. Added changelog entry. Added `__all__` to bounding_box.py Fixed more docstrings Integrated new bounding_box into automatic bounding_boxes Finished TODO to remove check on bounding box input processing. Improved test_custom_model_bounding_box test to test with_bounding_box=True Removed debugging print from a test. Added `__all__` to bounding_box.py Fixed more docstrings Refactored generic_call Removed generic_call from core.py Refactored _generic_evaluate into Model class Moved get_bounding_box into model class Moved _prepare_inputs_* functions into model class Refactored _prepare_outputs_* into model. Refactored validate_input_shapes into model Refactored some helper functions into model. Fixed codestyle errors. Added changelog entry. Minor refactoring and additional documentation. More documentation added. Fixed get_bounding_box in asdf Moved changelog to correct folder Fixed rebase issue Refactored and added tests to increase test coverage. Added initial new bounding box 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 Most changes to integrate new bounding box Integrated new bounding_box into automatic bounding_boxes Finished TODO to remove check on bounding box input processing. Updated _fix_input_bounding_box to be in line with PR astropy#11908. Made fix_inputs_with_bounding_box more robust Improved test_custom_model_bounding_box test to test with_bounding_box=True Removed debugging print from a test. Added changelog entry. Added `__all__` to bounding_box.py Fixed more docstrings Integrated new bounding_box into automatic bounding_boxes Finished TODO to remove check on bounding box input processing. Improved test_custom_model_bounding_box test to test with_bounding_box=True Removed debugging print from a test. Added `__all__` to bounding_box.py Fixed more docstrings Refactored generic_call Removed generic_call from core.py Refactored _generic_evaluate into Model class Moved get_bounding_box into model class Moved _prepare_inputs_* functions into model class Refactored _prepare_outputs_* into model. Refactored validate_input_shapes into model Refactored some helper functions into model. Fixed codestyle errors. Added changelog entry. Minor refactoring and additional documentation. More documentation added. Fixed get_bounding_box in asdf Moved changelog to correct folder Fixed rebase issue Refactored and added tests to increase test coverage. Most changes to integrate new bounding box Integrated new bounding_box into automatic bounding_boxes Finished TODO to remove check on bounding box input processing. Updated _fix_input_bounding_box to be in line with PR astropy#11908. Made fix_inputs_with_bounding_box more robust Improved test_custom_model_bounding_box test to test with_bounding_box=True Removed debugging print from a test. Added changelog entry. Added `__all__` to bounding_box.py Fixed more docstrings Integrated new bounding_box into automatic bounding_boxes Finished TODO to remove check on bounding box input processing. Improved test_custom_model_bounding_box test to test with_bounding_box=True Removed debugging print from a test. Added `__all__` to bounding_box.py Fixed more docstrings Refactored generic_call Removed generic_call from core.py Refactored _generic_evaluate into Model class Moved get_bounding_box into model class Moved _prepare_inputs_* functions into model class Refactored _prepare_outputs_* into model. Refactored validate_input_shapes into model Refactored some helper functions into model. Fixed codestyle errors. Added changelog entry. Minor refactoring and additional documentation. More documentation added. Fixed get_bounding_box in asdf Moved changelog to correct folder Fixed rebase issue Most changes to integrate new bounding box Integrated new bounding_box into automatic bounding_boxes Finished TODO to remove check on bounding box input processing. Updated _fix_input_bounding_box to be in line with PR astropy#11908. Made fix_inputs_with_bounding_box more robust Improved test_custom_model_bounding_box test to test with_bounding_box=True Removed debugging print from a test. Fixed unknown error in asdf round-trip for bounding_box Added `__all__` to bounding_box.py Integrated new bounding_box into automatic bounding_boxes Finished TODO to remove check on bounding box input processing. Improved test_custom_model_bounding_box test to test with_bounding_box=True Removed debugging print from a test. Fixed more docstrings Added changelog entry. Minor refactoring and additional documentation. More documentation added. Fixed get_bounding_box in asdf Integrated new bounding_box into automatic bounding_boxes Finished TODO to remove check on bounding box input processing. Improved test_custom_model_bounding_box test to test with_bounding_box=True Removed debugging print from a test. Fixed unknown error in asdf round-trip for bounding_box Added `__all__` to bounding_box.py Fixed more docstrings Minor refactoring and additional documentation. More documentation added. Fixed get_bounding_box in asdf Fixed rebase issue Minor refactoring and additional documentation. More documentation added. Moved changelog to correct folder Added changelog entry. Minor refactoring and additional documentation. More documentation added. Moved changelog to correct folder Added changelog entry. Minor refactoring and additional documentation. More documentation added. Minor refactoring and additional documentation. More documentation added. Minor refactoring and additional documentation. More documentation added. Introduced ignored bounding box inputs Added slice argument handling for compound bounding box Cleaned up rebase Factored evaluation methods out of bounding box Finished documenting BoundingDomain class. Added in basic compound bounding box class Integrated compound bounding box partially into core.py Finished integrating CompoundBoundingBox with core.py Finished adding tests for compound bounding box Added documentation for CompoundBoundingBox Cleaned up SliceArgument a bit Removed `add_ignored_axes` and related methods as these are no longer required. Added order notes to CompoundBoundingBox Added changelog entry. Updated model evaluation to be simpler Updated `__all__` in bounding_box.py Fixed a misspelling Moved changelog to correct place. Removed bad file. Updated docstrings Fixed more docstring errors. Removed bad file from docs. Fixed docstring serialize compound bounding box fix flake8 Mostly working fix_inputs Fixed broken fix_inputs cases Changed `slice_arg` to `selector_arg` and related throughout compound bounding box Addressed some of perrygreenfield's review comments. Fix for Azure test failure Updated asdf to used `selector` and `slice`. Intermediate work to diagnose issue Removed all the debugging statements. Increased test coverage. Fixed copy for models with a compound bounding box Fix for normal models Further fix for other mixed scalar-array cases. Changed bounding box tests to reflect changes to fix mixed scalar-array bug. Fixed copy bug with compound models

github-actions bot added 💤 io.misc.asdf Go to asdf-astropy repo modeling labels Jul 13, 2021

WilliamJamieson changed the title ~~Feature/compound bounding box~~ Adding CompoundBoundingBox Jul 13, 2021

perrygreenfield requested review from nden and perrygreenfield July 14, 2021 14:31

stscijgbot-jp mentioned this pull request Jul 14, 2021

Use spectral order as input to WCS in NIRISS SOSS mode spacetelescope/jwst#6216

Closed

WilliamJamieson force-pushed the feature/compound_bounding_box branch 3 times, most recently from ba3f8f9 to 6872d95 Compare July 19, 2021 18:36

WilliamJamieson force-pushed the feature/compound_bounding_box branch 5 times, most recently from ced9a35 to 398a3f4 Compare July 26, 2021 14:08

WilliamJamieson force-pushed the feature/compound_bounding_box branch 4 times, most recently from 1b62f1a to bba32e0 Compare July 30, 2021 16:02

WilliamJamieson force-pushed the feature/compound_bounding_box branch 2 times, most recently from 106c51f to 789ae6d Compare September 17, 2021 20:18

WilliamJamieson force-pushed the feature/compound_bounding_box branch from 789ae6d to a0cd416 Compare October 20, 2021 19:43

WilliamJamieson mentioned this pull request Oct 22, 2021

Enable Compound Bounding Box support spacetelescope/gwcs#375

Merged

WilliamJamieson force-pushed the feature/compound_bounding_box branch 2 times, most recently from 069b3d5 to 06371ae Compare October 25, 2021 18:49

WilliamJamieson mentioned this pull request Oct 25, 2021

Model evaluation refactor #11931

Merged

9 tasks

WilliamJamieson force-pushed the feature/compound_bounding_box branch from 06371ae to f69aa8d Compare October 26, 2021 13:07

nden added this to the v5.0 milestone Oct 26, 2021

WilliamJamieson force-pushed the feature/compound_bounding_box branch from f3d597d to 61c1f2e Compare October 26, 2021 16:39

WilliamJamieson force-pushed the feature/compound_bounding_box branch from 61c1f2e to 049cf68 Compare October 27, 2021 13:14

perrygreenfield reviewed Oct 27, 2021

View reviewed changes

WilliamJamieson force-pushed the feature/compound_bounding_box branch 2 times, most recently from 8ce5c07 to bf5a997 Compare October 28, 2021 02:07

nden reviewed Oct 28, 2021

View reviewed changes

astropy/io/misc/asdf/tags/transform/basic.py Outdated Show resolved Hide resolved

WilliamJamieson force-pushed the feature/compound_bounding_box branch 7 times, most recently from 2047992 to 5df6fb0 Compare October 29, 2021 17:11

WilliamJamieson force-pushed the feature/compound_bounding_box branch 3 times, most recently from 372f801 to 7291c7f Compare October 29, 2021 20:37

nden approved these changes Oct 29, 2021

View reviewed changes

WilliamJamieson force-pushed the feature/compound_bounding_box branch from 7291c7f to a993f61 Compare October 29, 2021 21:54

WilliamJamieson mentioned this pull request Oct 29, 2021

add unit-aware quantity annotations. #10662

Merged

WilliamJamieson force-pushed the feature/compound_bounding_box branch from a993f61 to afc3f0f Compare October 30, 2021 00:07

nden merged commit ccc48ee into astropy:main Oct 30, 2021

nden mentioned this pull request Oct 30, 2021

Add support for "complex" bounding boxes. #11699

Closed

WilliamJamieson deleted the feature/compound_bounding_box branch October 30, 2021 01:53

This was referenced Nov 1, 2021

Error in bounding box when running with mixed scalar and array arguments. #12319

Closed

Cleanup of recent modeling changes #12343

Merged

Updates to documentation for new modeling bounding box features. #12364

Merged

larrybradley mentioned this pull request Nov 2, 2021

Bug in CompoundModel copy() #12370

Closed

eslavich mentioned this pull request Nov 3, 2021

Catch up to changes in ASDF support in astropy 5.0 astropy/asdf-astropy#29

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding `CompoundBoundingBox` #11942

Adding `CompoundBoundingBox` #11942

WilliamJamieson commented Jul 13, 2021 •

edited

pep8speaks commented Jul 19, 2021 •

edited

WilliamJamieson commented Oct 26, 2021 •

edited

perrygreenfield left a comment

WilliamJamieson commented Oct 29, 2021

nden commented Oct 29, 2021

Adding CompoundBoundingBox #11942

Adding CompoundBoundingBox #11942

Conversation

WilliamJamieson commented Jul 13, 2021 • edited

Description

Checklist for package maintainer(s)

pep8speaks commented Jul 19, 2021 • edited

Comment last updated at 2021-10-30 00:07:20 UTC

WilliamJamieson commented Oct 26, 2021 • edited

perrygreenfield left a comment

Choose a reason for hiding this comment

WilliamJamieson commented Oct 29, 2021

nden commented Oct 29, 2021

Adding `CompoundBoundingBox` #11942

Adding `CompoundBoundingBox` #11942

WilliamJamieson commented Jul 13, 2021 •

edited

pep8speaks commented Jul 19, 2021 •

edited

WilliamJamieson commented Oct 26, 2021 •

edited