Clean up bounding box assignment

[WilliamJamieson]

spacetelescope/gwcs#522 adds a warning to flag instances where bounding boxes that GWCS uses are ordered using the C (y, x) ordering rather than the F ordering the GWCS defaults to. GWCS handles the alternate ordering by default however tuple assignment to the forward transform before creating the GWCS causes astropy to assume a C ordering while if one assigns the same tuple directly to the wcs, GWCS will assume a F order. Due to this source of issues GWCS now throws a warning when it discovers an assigned bounding box with the C ordering as GWCS will always attach and handle bounding boxes specifically in the F ordering.

This PR updates romancal so that bounding boxes are always attached with the tuple correctly ordered and flagged as F order.

Tasks

• request a review from someone specific, to avoid making the maintainers review every PR
• add a build milestone, i.e. 24Q4_B15 (use the latest build if not sure)
• Does this PR change user-facing code / API? (if not, label with no-changelog-entry-needed)
  • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types)
  • update or add relevant tests
  • update relevant docstrings and / or docs/ page
  • start a regression test and include a link to the running job (click here for instructions)
    • Do truth files need to be updated ("okified")?
      • after the reviewer has approved these changes, run okify_regtests to update the truth files
• if a JIRA ticket exists, make sure it is resolved properly

news fragment change types...

• changes/<PR#>.general.rst: infrastructure or miscellaneous change
• changes/<PR#>.docs.rst
• changes/<PR#>.stpipe.rst
• changes/<PR#>.associations.rst
• changes/<PR#>.scripts.rst
• changes/<PR#>.mosaic_pipeline.rst
• changes/<PR#>.patch_match.rst

steps

• changes/<PR#>.dq_init.rst
• changes/<PR#>.saturation.rst
• changes/<PR#>.refpix.rst
• changes/<PR#>.linearity.rst
• changes/<PR#>.dark_current.rst
• changes/<PR#>.jump_detection.rst
• changes/<PR#>.ramp_fitting.rst
• changes/<PR#>.assign_wcs.rst
• changes/<PR#>.flatfield.rst
• changes/<PR#>.photom.rst
• changes/<PR#>.flux.rst
• changes/<PR#>.source_detection.rst
• changes/<PR#>.tweakreg.rst
• changes/<PR#>.skymatch.rst
• changes/<PR#>.outlier_detection.rst
• changes/<PR#>.resample.rst
• changes/<PR#>.source_catalog.rst

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.68%. Comparing base (281aa55) to head (2430655).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1527      +/-   ##
==========================================
- Coverage   76.68%   76.68%   -0.01%     
==========================================
  Files         120      120              
  Lines        7832     7831       -1     
==========================================
- Hits         6006     6005       -1     
  Misses       1826     1826              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests

[WilliamJamieson]

Regression tests pass. Note that these tests were specifically run with pytest turning all the bounding box warnings from GWCS into errors. https://github.com/spacetelescope/RegressionTests/actions/runs/11916504320

[mairanteodoro]

Looks good to me.

[schlafly]

Looks good to me.

FWIW, bind_bounding_box is new to me. Would you mind saying a few words about when this is needed? i.e., in parts of this PR you do wcs_obj.bounding_box = box, but for adding it to the transform you use the bind_bounding_box routine rather than the previous simpler-looking direct assignment. Relatedly, it looks like when you assign to the wcs object you don't specify an order (presumably 'F' is assumed), but you are more explicit when assigning to the transform. What motivates being more explicit in one case than in the other?

[WilliamJamieson]

The use of tuples of tuples to represent bounding boxes for 2D+ circumstances has annoying consequences due to choices within astropy. For astropy models, when setting a bounding box via the tuple of tuples method, bounding boxes are assumed to be ordered in reverse input order, i.e. if the model has inputs ordered x, y then then it must be the case that for assignment

model.bounding_box = ((y_min, y_max), (x_min, x_max))

This is the so called "C" ordering. By contrast, GWCS assumes the bounding box is ordered in the coordinate input ordering, so if they are say ra, dec then for assignment

wcs.bounding_box = ((ra_min, ra_max), (dec_min, dec_max))

This is the so called "F" ordering. Hopefully it is clear that when building a WCS using GWCS it can be very very easy to accidentally reverse a bounding box from what you meant. There are two possible mistakes

1. Building the bounding box using the GWCS assumed ordering, but attaching it to the forward transform before passing the transform to the WCS.
2. Building the bounding box using the assumed astropy ordering, but attaching it to the final WCS.

In either case, one ends up with the bounding box in reverse order to what it should be. The bugs resulting from this can be rather difficult to find (ask me how I know), especially when the box is nearly but not total square. Hence, GWCS's new attempts to warn users that there maybe a possible issue.

Note that GWCS uses the "first" forward_transform to store the WCS's bounding box. Since this is an astropy model, everything ultimately gets passed into astropy. astropy internally handles bounding boxes not as tuples of tuples, but as abstracted objects which associate an "interval" (min/max) with each input variable. This means that programmatically once a bounding box is attached to a model, the "ordering" does not matter; however, it does matter when using tuples of tuples to set the bounding box. There are several methods for actively setting a bounding box to an astropy model using the F ordering, they just do not involve the model.bounding_box =. Once such method is the bind_bounding_box function, which exists as a simple way to accomplish using a F ordered tuple to attach a bounding box to them model and indicate that it was an F ordered box to start with, which is how GWCS implements the wcs.bounding_box= operation.

The method GWCS uses for issuing the warning for a possible bounding box issue is interrogation of the bounding box's order when it grabs said bounding box from the first forward transform. If that order is '"C"' (is dimension > 2, and not a default bounding box from the model class itself), GWCS knows that the bounding box was set outside of its control meaning the user had to remember to reverse it.

Looks good to me.

FWIW, bind_bounding_box is new to me. Would you mind saying a few words about when this is needed? i.e., in parts of this PR you do wcs_obj.bounding_box = box, but for adding it to the transform you use the bind_bounding_box routine rather than the previous simpler-looking direct assignment. Relatedly, it looks like when you assign to the wcs object you don't specify an order (presumably 'F' is assumed), but you are more explicit when assigning to the transform. What motivates being more explicit in one case than in the other?

As to why bind_bounding_box was used in some places while in others I moved the bounding box attachment around? If you look closely bind_bounding_box is used in cases where the forward transform is being built along side the bounding box separate from where the WCS is built, ie in a function that generates the forward transform, which is then passed to the WCS. In these cases, it is rather awkward to set the bounding box directly on the WCS instead, so the bind_bounding_box is used to explicitly set the bounding box using the "F" ordering. In the cases, where I moved bounding box assignment around, were those where the bounding box object was generated in the same context as the WCS was being built, which means I could simply reverse the bounding box tuple order and then attach it to the WCS directly (from which GWCS would then do all the necessary stuff under the hood to make sure it was attached as an order "F" bounding box.

Ideally, the bounding box should always be computed in such a way we can pass directly to the transform; however, that was not always the easiest thing to do without more significant changes. I am planning to clean up all the WCS stuff in romancal once we have finalized the GWCS API for GWCS release 1.0, so making significant code changes now to do stuff more cleanly does not really make sense to do significant changes in this PR.

[WilliamJamieson]

This is a case where the GWCS warning identified a bug. It just did not get picked up by any tests...

Note

romancal/romancal/assign_wcs/utils.py

Lines 20 to 35 in 281aa55

	def wcs_bbox_from_shape(shape):
	"""Create a bounding box from the shape of the data.
	This is appropriate to attach to a wcs object.
	Parameters
	----------
	shape : tuple
	The shape attribute from a `numpy.ndarray` array.
	Returns
	-------
	bbox : tuple
	Bounding box in x, y order.
	"""
	bbox = ((-0.5, shape[-1] - 0.5), (-0.5, shape[-2] - 0.5))
	return bbox

Reports to create a bounding box in the x, y ordering but the transform.bounding_box = was making the assumption this was a y, x box.

With this change it is now being assigned correctly with an x, y ordering.

[schlafly]

Ah, good, astropy.modeling uses the other convention and has this box convention---thanks, that makes sense.

As to why bind_bounding_box was used in some places while in others I moved the bounding box attachment around? If you look closely bind_bounding_box is used in cases where the forward transform is being built along side the bounding box separate from where the WCS is built, ie in a function that generates the forward transform, which is then passed to the WCS.

Yup, great, this makes sense, thanks!