-
Notifications
You must be signed in to change notification settings - Fork 32
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
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. |
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.
@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?
f43c2ab
to
bae0418
Compare
I did not run regression tests (or real data) since unit tests were failing.
I am not sure about the root-cause, but it appears (to me) that the issue is only with some instruments. In any case, |
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 |
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
|
But I do agree it may be hiding something. |
I want to do more testing before we merge this. |
Fixes failures in JWST pipeline due to GWCS inverse transformation enforcing bounding box.
Tasks
docs/
pageno-changelog-entry-needed
)changes/
:echo "changed something" > changes/<PR#>.<changetype>.rst
(see below for change types)"git+https://github.com/<fork>/stcal@<branch>"
)jwst
regression testromancal
regression testnews fragment change types...
changes/<PR#>.apichange.rst
: change to public APIchanges/<PR#>.bugfix.rst
: fixes an issuechanges/<PR#>.general.rst
: infrastructure or miscellaneous change