8000 JP-4021: Fix a bug where insufficient valid background pixels case was not caught for WFSS background step by emolter · Pull Request #9507 · spacetelescope/jwst · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

JP-4021: Fix a bug where insufficient valid background pixels case was not caught for WFSS background step #9507

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Jun 5, 2025
Merged

JP-4021: Fix a bug where insufficient valid background pixels case was not caught for WFSS background step #9507

merged 7 commits into from
Jun 5, 2025

Conversation

emolter
Copy link
Collaborator
@emolter emolter commented Jun 2, 2025 •
edited
Loading

Resolves JP-4021

Closes #9504

This PR fixes a bug in the background step for WFSS data caused by the interaction between a mostly-masked input image and a reference background with a substantial region of zero values. See the JIRA ticket for a detailed description of the cause of the bug. This PR updates the _sufficient_background_pixels function to account for not only the number of masked pixels, but instead the number of masked pixels for which the background reference file is not identically zero.

Tasks

  • If you have a specific reviewer in mind, tag them.
  • add a build milestone, i.e. Build 12.0 (use the latest build if not sure)
  • Does this PR change user-facing code / API? (if not, label with no-changelog-entry-needed)
    • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst 8000 (see changelog readme for instructions)
    • update or add relevant tests
    • update relevant docstrings and / or docs/ page
    • start a regression test and include a link to the running job (click here for instructions)
      • Do truth files need to be updated ("okified")?
        • after the reviewer has approved these changes, run okify_regtests to update the truth files
  • if a JIRA ticket exists, make sure it is resolved properly

@emolter emolter added this to the Build 12.0 milestone Jun 2, 2025
@emolter
Copy link
Collaborator Author
emolter commented Jun 2, 2025 •
edited
Loading

https://github.com/spacetelescope/RegressionTests/actions/runs/15399730756

all passing

@emolter emolter marked this pull request as ready for review June 4, 2025 14:12
@emolter emolter requested review from a team as code owners June 4, 2025 14:12
@emolter
Copy link
Collaborator Author
emolter commented Jun 4, 2025

@gnoir0t does this look ok to you? For some reason I can't request you as a reviewer

@codecov Codecov
Copy link
codecov bot commented Jun 4, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 66.66667% with 4 lines in your changes missing coverage. Please review.

Project coverage is 78.47%. Comparing base (6f304b2) to head (132c0de).
Report is 16 commits behind head on main.

Files with missing lines Patch % Lines
jwst/background/background_sub_wfss.py 66.66% 4 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #9507      +/-   ##
==========================================
- Coverage   78.47%   78.47%   -0.01%     
==========================================
  Files         361      361              
  Lines       36565    36574       +9     
==========================================
+ Hits        28696    28701       +5     
- Misses       7869     7873       +4

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@pllim
Copy link
Collaborator
pllim commented Jun 4, 2025

If STScI staff has read access, then the person of interest need to be part of that GH Team. Otherwise, it is recommended we add STScI staff to collaborators with read access only to be able to request any staff member to review. I am unable to check that setting with my access. Hope this helps.

@emolter
Copy link
Collaborator Author
emolter commented Jun 4, 2025

If STScI staff has read access, then the person of interest need to be part of that GH Team. Otherwise, it is recommended we add STScI staff to collaborators with read access only to be able to request any staff member to review. I am unable to check that setting with my access. Hope this helps.

Let me rephrase: I know the reason, in a technical sense, why I can't request reviews from certain people. I do not know the procedure (if any yet exists) by which I should try to give people that permission.

melanieclarke
melanieclarke reviewed Jun 4, 2025
View reviewed changes
Copy link
Collaborator
@melanieclarke melanieclarke left a comment

Choose a reason for hiding this comment

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

LGTM for catching this specific problem, but should we add a safety catch for scale_factor = NaN, just in case some other problem arises we haven't anticipated?

@emolter
Copy link
Collaborator Author
emolter commented Jun 4, 2025

part of me wants to say that if things don't work for reasons we don't understand then it should crash. But I can put the check in if you like

@melanieclarke
Copy link
Collaborator

part of me wants to say that if things don't work for reasons we don't understand then it should crash. But I can put the check in if you like

Yes, please. I think it shouldn't crash for an unforeseen numerical error, but even if it should crash, it shouldn't do so many steps later when we try to save the FITS file.

@emolter emolter requested a review from melanieclarke June 5, 2025 13:30
melanieclarke
melanieclarke approved these changes Jun 5, 2025
View reviewed changes
Copy link
Collaborator
@melanieclarke melanieclarke left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

It looks to me like this implementation is as requested on the ticket, so I think we can go ahead and merge and let INS test from there.

@emolter emolter enabled auto-merge (squash) June 5, 2025 13:42
@emolter emolter merged commit e058bfe into spacetelescope:main Jun 5, 2025
19 checks passed
@stscijgbot-jp stscijgbot-jp mentioned this pull request Jun 5, 2025
Closed
@zacharyburnett
Copy link
Collaborator

@meeseeksdev backport to release/1.18.x

@lumberbot-app Lumberbot (App)
Copy link
lumberbot-app bot commented Jun 9, 2025

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout release/1.18.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 e058bfe1f28641fbc45508f7ecffb7b05c6a239c
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #9507: JP-4021: Fix a bug where insufficient valid background pixels case was not caught for WFSS background step'
  1. Push to a named branch:
git push YOURFORK release/1.18.x:auto-backport-of-pr-9507-on-release/1.18.x
  1. Create a PR against branch release/1.18.x, I would have named this PR:

"Backport PR #9507 on branch release/1.18.x (JP-4021: Fix a bug where insufficient valid background pixels case was not caught for WFSS background step)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

zacharyburnett pushed a commit that referenced this pull request Jun 9, 2025
@emolter @zacharyburnett
JP-4021: Fix a bug where insufficient valid background pixels case wa…
8b00f8f 
…s not caught for WFSS background step (#9507)

(cherry picked from commit e058bfe)
zacharyburnett pushed a commit to zacharyburnett/jwst that referenced this pull request Jun 9, 2025
@emolter @zacharyburnett
Backport PR spacetelescope#9507: JP-4021: Fix a bug where insufficien…
1d7bf37 
…t valid background pixels case was not caught for WFSS background step
@zacharyburnett zacharyburnett mentioned this pull request Jun 9, 2025
Merged
zacharyburnett pushed a commit to zacharyburnett/jwst that referenced this pull request Jun 10, 2025
@emolter @zacharyburnett
Backport PR spacetelescope#9507: JP-4021: Fix a bug where insufficien…
b0991a8 
…t valid background pixels case was not caught for WFSS background step
zacharyburnett added a commit that referenced this pull request Jun 10, 2025
@zacharyburnett @emolter
Backport PR #9507: JP-4021: Fix a bug where insufficient valid backgr…
b92e536 
…ound pixels case was not caught for WFSS background step (#9528)

Co-authored-by: Ned Molter <emolter@users.noreply.github.com>
@gnoir0t
Copy link
gnoir0t commented Jun 10, 2025 •
edited
Loading

@emolter For the record I made some tests of the PR using an example NIRISS WFSS exposure from PID 1089 and it seems to work as expected. I first tested running the background step with default parameters which ran as expected, and then I ran the same test but I modified the input DQ array to artificially create an exposure with >=95% of non valid pixels, and the background step was skipped as expected.
The only improvement I could see is that the code uses 5% of 2048x2048 (the size of the DQ array) as the threshold of valid and non-masked pixels, which is the most conservative case. In principle it could be 2048x2048 - reference_pixels - bad_pixels (which Russ also suggested). But it's probably okay for now.

@emolter emolter deleted the JP-4021 branch June 10, 2025 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@melanieclarke melanieclarke melanieclarke approved these changes

Assignees
No one assigned
Labels
background testing Wide Field Slitless Spec (WFSS)
Projects
None yet
Milestone
Build 12.0
Development

Successfully merging this pull request may close these issues.

program 1187 nrc_wfss - crashed in extract_1d
5 participants
@emolter @pllim @melanieclarke @zacharyburnett @gnoir0t
0