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

JP-3604: Fix possible crashes in the alignment to abs catalog #8450

Merged
merged 1 commit into from May 9, 2024

Conversation

mcara
Copy link
Member

@mcara mcara commented Apr 30, 2024

Resolves JP-3604

Closes #8435

This PR improves handling of exceptions in the absolute alignment stage that could result in crashes when image catalogs contain no sources.

Checklist for maintainers

  • added entry in CHANGES.rst within the relevant release section
  • updated or added relevant tests
  • updated relevant documentation
  • added relevant milestone
  • added relevant label(s)
  • ran regression tests, post a link to the Jenkins job below.
    How to run regression tests on a PR
  • Make sure the JIRA ticket is resolved properly

Regression test: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1410/

@mcara mcara added the tweakreg label Apr 30, 2024
@mcara mcara added this to the Build 10.0.1 milestone Apr 30, 2024
@mcara mcara self-assigned this Apr 30, 2024
@mcara mcara requested a review from a team as a code owner April 30, 2024 05:17
Copy link

codecov bot commented Apr 30, 2024

Codecov Report

Attention: Patch coverage is 17.24138% with 24 lines in your changes are missing coverage. Please review.

Project coverage is 56.50%. Comparing base (6580914) to head (39bbe84).
Report is 13 commits behind head on master.

Files Patch % Lines
jwst/tweakreg/tweakreg_step.py 17.24% 24 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8450      +/-   ##
==========================================
+ Coverage   56.38%   56.50%   +0.12%     
==========================================
  Files         387      387              
  Lines       38716    38837     +121     
==========================================
+ Hits        21830    21945     +115     
- Misses      16886    16892       +6     

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

@hbushouse hbushouse modified the milestones: Build 10.0.1, Build 11.0 Apr 30, 2024
@hbushouse
Copy link
Collaborator

@mcara Have you been able to get the data that caused this crash in Ops, in order to test your proposed fix, or do you still need that?

Copy link
Collaborator

@mairanteodoro mairanteodoro left a comment

Choose a reason for hiding this comment

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

Thanks, @mcara! It looks good to me, but I added a suggestion to make the code DRYer. ;)

jwst/tweakreg/tweakreg_step.py Show resolved Hide resolved
Copy link
Collaborator

@nden nden left a comment

Choose a reason for hiding this comment

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

Looks good and fixes the issue.
It's good to add this as a regression test using the data that crashed.

@hbushouse
Copy link
Collaborator

Are we OK to merge this as-is, or do you want to add a regression test to this PR?

@hbushouse
Copy link
Collaborator

I grabbed the data made available from DMS, which is now in /grp/jwst/ssb/chartreuse/dms_bugs/jp-3604/, and I can't get it fail whether I run it with B10.0, B10.1, or the latest 11.0 dev version. So it's really hard to know what impact this PR will have (i.e. whether it really fixes anything or not).

@mcara
Copy link
Member Author

mcara commented May 9, 2024

I think the issue may be in that they changed the reference parameter file for this step in CRDS. They changed abs_refcat from '' to 'GAIADR3'. Previously absolute alignment was disabled by default. Try re-running old release with the latest ref from CRDS.

@mcara mcara force-pushed the catch-exceptions-abs-align branch from 89d8f34 to 39bbe84 Compare May 9, 2024 04:13
@mcara
Copy link
Member Author

mcara commented May 9, 2024

@hbushouse Unit test could be added. However, if #8476 is approved, it wouldn't be possible to trigger this crash in xyxymatch. As I was typing this, maybe #8476 should have been be part of this PR but it is OK to have 2 PRs too.

@mcara
Copy link
Member Author

mcara commented May 9, 2024

So I think it is OK to merge this PR and then #8476.

@nden
Copy link
Collaborator

nden commented May 9, 2024

I reproduced the crash and this PR fixes it. Try passing abs_refcat='GAIADR3 when running tweakreg.

@hbushouse
Copy link
Collaborator

Ah yes, of course. When running with tweakreg.abs_refcat="GAIADR3" I can reproduce the failure.

@hbushouse hbushouse merged commit 55098d8 into spacetelescope:master May 9, 2024
26 of 28 checks passed
@nden
Copy link
Collaborator

nden commented May 13, 2024

A regression test was added in #8477

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

Successfully merging this pull request may close these issues.

skipped tweakwcs step during niriss image3 got restarted anyway
4 participants