-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set the bounding_box within the forward_transform property #532
Set the bounding_box within the forward_transform property #532
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #532 +/- ##
==========================================
+ Coverage 87.42% 87.45% +0.02%
==========================================
Files 22 22
Lines 3874 3891 +17
==========================================
+ Hits 3387 3403 +16
- Misses 487 488 +1 ☔ View full report in Codecov by Sentry. |
# Currently compound models do not attempt to combine individual model | ||
# bounding boxes. Get the forward transform and assign the bounding_box to it | ||
# before evaluating it. The order Model.bounding_box is reversed. | ||
transform.bounding_box = self.bounding_box |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remind me why is it not necessary to reverse the bounding box when assigning to a transform? Is the default in modeling now order=F
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines 1282 to 1312 in c01c6d0
def bounding_box(self): | |
""" | |
Return the range of acceptable values for each input axis. | |
The order of the axes is `~gwcs.coordinate_frames.CoordinateFrame.axes_order`. | |
""" | |
frames = self.available_frames | |
transform_0 = self.get_transform(frames[0], frames[1]) | |
try: | |
bb = transform_0.bounding_box | |
except NotImplementedError: | |
return None | |
if ( | |
# Check that the bounding_box was set on the instance (not a default) | |
transform_0._user_bounding_box is not None | |
# Check the order of that bounding_box is C | |
and bb.order == "C" | |
# Check that the bounding_box is not a single value | |
and (isinstance(bb, CompoundBoundingBox) or len(bb) > 1) | |
): | |
warnings.warn( | |
"The bounding_box was set in C order on the transform prior to being used in the gwcs!\n" | |
"Check that you indended that ordering for the bounding_box, and consider setting it in F order.\n" | |
"The bounding_box will remain meaning the same but will be converted to F order for consistency in the GWCS.", | |
GwcsBoundingBoxWarning | |
) | |
self.bounding_box = bb.bounding_box(order="F") | |
bb = self.bounding_box | |
return bb |
Returns a astropy.modeling.bounding_box.ModelBoundingBox
not a tuple. That full object will be processed correctly by astropy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regression tests have the same failures as main.
https://github.com/spacetelescope/RegressionTests/actions/runs/12168121751
Should we be more explicit in setting the bounding_box in assign_wcs, e.g. here? |
Astropy modeling does not attempt to determine the bounding box for compound models. Thus when the
wcs.forward_transform
computes the transform model by chaining all thewcs
transforms together the bounding box is lost. The bounding box for thewcs.forward_transform
should always be thebounding_box
for thewcs
. Where ever theforward_transform
is actually used the bounding box is manually attached.This PR moves that attachment inside the
forward_transform
property itself.