Skip to content
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

Ignore new bounding box order warning from GWCS #3275

Closed
wants to merge 1 commit into from

Conversation

rosteen
Copy link
Collaborator

@rosteen rosteen commented Nov 8, 2024

This is needed for dev tests to pass due to spacetelescope/gwcs#522. I think we can safely ignore this warning here.

kecnry
kecnry previously approved these changes Nov 8, 2024
@WilliamJamieson
Copy link

This warning is in-place because attaching the bounding box to the first transform before passing it to the GWCS can have very different behavior for when it is attached to the GWCS directly. Essentially for 2D+ bounding boxes astropy interprets (by default) these tuples in reverse order of inputs, i.e. it believes ((y_min, y_max), (x_min, x_max)) where as GWCS interprets the same tuple in the order of inputs, i.e. it believes ((x_min, x_max), (y_min, y_max)). So you need to check which portion of the tuple means what if your code assigns the bounding box prior to passing the transform to GWCS. Note it is possible to set the bounding box on the transform with the GWCS interpretation but it cannot be done using the standard interface.

I see the failing test appears to be one where you are loading GWCS information from a file. It is worth investigating how that GWCS received its bounding box, and if necessary check the bounding box dimensional assumptions. Note that if the file is pretty old, the bounding box will always be assumed by ASDF to have the astropy ordering in the way it is recorded. Newer versions remove the possibility of this ambiguity and will add the ordering indication flag. This will remove the warning.

If your sure the bounding box is correctly added in your current file simply open the file and then access the bounding box through the GWCS object, upon first access GWCS will recast in place the ordering indication flag to the correct one for it without changing the meaning of which bound applies to which input. Then resave your file, this will also eliminate the warning.

@kecnry kecnry self-requested a review November 8, 2024 19:20
@kecnry kecnry dismissed their stale review November 8, 2024 19:21

looks like this either needs logic to ignore warning based on version or updating the file instead

@pllim
Copy link
Contributor

pllim commented Nov 11, 2024

Wait a minute. We have to do something besides ignoring it, right? Isn't the whole Orientation plugin written by @bmorris3 based on GWCS and its bounding boxes?

Also seems to be used here:

https://github.com/search?q=repo%3Aspacetelescope%2Fjdaviz%20bounding_box&type=code

@pllim
Copy link
Contributor

pllim commented Nov 12, 2024

So, I checked our code. We already assumed F-order anyway so the warning does not really apply. It seems to blindly check whenever bounding_box is accessed, which means, our parsers are affected. Since we cannot go and fix the files on MAST, I think it is okay to ignore. I folded it in over at #3283 because it should be localized, just in case the warning becomes real somewhere else down the line.

I also asked follow-up questions at spacetelescope/gwcs#522 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants