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

Fix issues in outlier det. due to GWCS.inverse enforcing bbox #324

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mcara
Copy link
Member

@mcara mcara commented Dec 16, 2024

Fixes failures in JWST pipeline due to GWCS inverse transformation enforcing bounding box.

Tasks

  • update or add relevant tests
  • update relevant docstrings and / or docs/ page
  • Does this PR change any API used downstream? (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)
    • run regression tests with this branch installed ("git+https://github.com/<fork>/stcal@<branch>")
news fragment change types...
  • changes/<PR#>.apichange.rst: change to public API
  • changes/<PR#>.bugfix.rst: fixes an issue
  • changes/<PR#>.general.rst: infrastructure or miscellaneous change

@mcara mcara requested a review from a team as a code owner December 16, 2024 22:14
@mcara mcara self-assigned this Dec 16, 2024
@mcara mcara added the bug Something isn't working label Dec 16, 2024
@mcara mcara requested review from nden and emolter December 16, 2024 22:14
Copy link

codecov bot commented Dec 16, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.

Project coverage is 86.88%. Comparing base (728b877) to head (8838fb7).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/stcal/outlier_detection/utils.py 85.71% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #324       +/-   ##
===========================================
+ Coverage   29.56%   86.88%   +57.32%     
===========================================
  Files          37       49       +12     
  Lines        8332     8943      +611     
===========================================
+ Hits         2463     7770     +5307     
+ Misses       5869     1173     -4696     

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

Copy link
Collaborator

@emolter emolter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcara @nden Did this failure show up anywhere besides the outlier detection unit tests? e.g., in the regression tests or with any real data? The outlier detection tests were not calling AssignWcsStep properly and fixing that seemed to resolve the problem without any code changes outside the test itself: https://github.com/nden/jwst/pull/1/files

Is there a good reason to explicitly ignore the bounding box when calling reproject()? It seems to me that if the bounding box is reasonable, then the proposed change should not be necessary. The bounding box was not reasonable for the outlier tests, and calling AssignWcs made it reasonable and made the tests pass. I wonder if something similar could be happening for the resample unit test?

@mcara mcara force-pushed the fix-gwcs-inv-bbox branch from f43c2ab to bae0418 Compare December 16, 2024 22:26
@mcara
Copy link
Member Author

mcara commented Dec 16, 2024

Did this failure show up anywhere besides the outlier detection unit tests? e.g., in the regression tests or with any real data?

I did not run regression tests (or real data) since unit tests were failing.

Is there a good reason to explicitly ignore the bounding box when calling reproject()? It seems to me that if the bounding box is reasonable, then the proposed change should not be necessary. The bounding box was not reasonable for the outlier tests, and calling AssignWcs made it reasonable and made the tests pass. I wonder if something similar could be happening for the resample unit test?

I am not sure about the root-cause, but it appears (to me) that the issue is only with some instruments. In any case, drizzle can filter out pixels that are outside of the output frame by itself (without much overhead) and it can also handle NaN in pixmap. However, it cannot handle the case when ALL values in pixmap are NaN which is what the failures were in many unit tests.

@emolter
Copy link
Collaborator

emolter commented Dec 16, 2024

I am not sure about the root-cause, but it appears (to me) that the issue is only with some instruments. In any case, drizzle can filter out pixels that are outside of the output frame by itself (without much overhead) and it can also handle NaN in pixmap. However, it cannot handle the case when ALL values in pixmap are NaN which is what the failures were in many unit tests.

Why does it appear to you that the failures are instrument-specific?

The all-NaN pixmaps were an effect of a bad input WCS, which led reproject() to return NaN no matter what was input. As I said, calling AssignWcsStep prior to running the test (instead of assign_wcs.pointing.create_fitswcs) solves the problem for the outlier unit tests. I don't think an all-NaN pixmap should happen unless the input WCS is not correct for the input data. It seems like the proposed fix just hides that pre-existing problem.

@mcara
Copy link
Member Author

mcara commented Dec 17, 2024

Why does it appear to you that the failures are instrument-specific?

Maybe I misspoke. I just saw some of the regression tests passing and others not. Quite possibly my conclusion was wrong.

However, I do not see any benefit in using bounding_box for the inverse transformation for pixmap calculation specifically if the intension is to use it for resample/drizzle:

  1. drizzle computes intersections between input and output images and tries to sample input images only where input pixels would map onto output frame.
  2. drizzle checks that output coordinate does not land outside the output array;
  3. enabling bounding box on the inverse transform will only slowdown computations.

@mcara
Copy link
Member Author

mcara commented Dec 17, 2024

But I do agree it may be hiding something.

@nden
Copy link
Collaborator

nden commented Dec 17, 2024

I want to do more testing before we merge this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working no-changelog-entry-needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants