8000 JP-3830: MRS residual cosmic ray shower straylight correction by drlaw1558 · Pull Request #9126 · spacetelescope/jwst · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

JP-3830: MRS residual cosmic ray shower straylight correction #9126

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 21 commits into from
Feb 6, 2025
Merged

JP-3830: MRS residual cosmic ray shower straylight correction #9126

merged 21 commits into from
Feb 6, 2025

Conversation

drlaw1558
Copy link
Collaborator
@drlaw1558 drlaw1558 commented Jan 31, 2025 •
edited by melanieclarke
Loading

Resolves JP-3830 by adding an additional optional step to the MIRI MRS straylight correction code to measure and remove residual cosmic ray showers using information from the inter-slice pixels.

Tasks

  • request a review from someone specific, to avoid making the maintainers review every PR
  • add a build milestone, i.e. Build 11.3 (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 (see below for change types)
    • 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

@drlaw1558 drlaw1558 requested review from a team as code owners January 31, 2025 20:10
@stscijgbot-jp stscijgbot-jp mentioned this pull request Jan 31, 2025
Closed
@codecov Codecov
Copy link
codecov bot commented < 8000 a href="#issuecomment-2628379331" id="issuecomment-2628379331-permalink" class="Link--secondary js-timestamp">Jan 31, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 62.16216% with 14 lines in your changes missing coverage. Please review.

Project coverage is 73.77%. Comparing base (d0d1a67) to head (8191748).
Report is 847 commits behind head on main.

Files with missing lines Patch % Lines
jwst/straylight/straylight.py 71.42% 8 Missing ⚠️
jwst/straylight/straylight_step.py 33.33% 6 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #9126      +/-   ##
==========================================
- Coverage   73.77%   73.77%   -0.01%     
==========================================
  Files         372      372              
  Lines       37265    37272       +7     
==========================================
+ Hits        27492    27497       +5     
- Misses       9773     9775       +2

☔ 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.

@melanieclarke melanieclarke added this to the Build 11.3 milestone Feb 4, 2025
@melanieclarke
Copy link
Collaborator
melanieclarke commented Feb 4, 2025 •
edited
Loading

I started regression tests here:
https://github.com/spacetelescope/RegressionTests/actions/runs/13145149831

It would be nice to have a unit test or two to cover the new code. Do you have bandwidth to give that a try?

Edit: regtest results look fine. There are a few unrelated errors, but no changes as a result of this PR, as expected. I'll run again when code changes are done.

@drlaw1558
Copy link
Collaborator Author

Good call; I've added in a simple test.

melanieclarke
melanieclarke reviewed Feb 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.

Thank you for the unit test, and all the documentation updates! I have a few software suggestions below.

drlaw1558 and others added 4 commits February 5, 2025 11:57
@drlaw1558 @melanieclarke
Update jwst/straylight/straylight.py
0c2264e 
Co-authored-by: Melanie Clarke <mclarke@stsci.edu>
@drlaw1558 @melanieclarke
Update jwst/straylight/straylight.py
e5eaeaa 
Co-authored-by: Melanie Clarke <mclarke@stsci.edu>
@drlaw1558 @melanieclarke
Update jwst/straylight/straylight.py
f3cf9a6 
Co-authored-by: Melanie Clarke <mclarke@stsci.edu>
@drlaw1558
Various changes based on review
bf5bd84
melanieclarke
melanieclarke reviewed Feb 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.

Thanks for the quick responses! I have a couple more cleanup requests, and I'll rerun regtests, but I think this looks great. I tested the correction with default parameters on some MRS regression test data, and the results are a significant improvement!

@melanieclarke
Copy link
Collaborator

Tagging @jemorrison for a second pair of eyes, if you have a little time to review.

drlaw1558 and others added 3 commits February 5, 2025 14:13
@drlaw1558 @melanieclarke
Update jwst/straylight/straylight.py
6e1274e 
Co-authored-by: Melanie Clarke <mclarke@stsci.edu>
@drlaw1558 @melanieclarke
Update jwst/straylight/straylight_step.py
d0d1a67 
Co-authored-by: Melanie Clarke <mclarke@stsci.edu>
@melanieclarke
Merge branch 'main' into JP-3830
bdd2a34
jemorrison
jemorrison approved these changes Feb 6, 2025
View reviewed changes
Copy link
Collaborator
@jemorrison jemorrison 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 to me. One tiny comment on doc.
The clean_showers module is surprising simple considering what it is trying to remove.

I was surprised at the large number for shower_x_stddev (18.0). Just for my understanding why is this set so large? The bad pixels have been naned out and the 3rd plane of the regions file is use to mark pixels that receive light from the sky and as I understand the shower_low and high reject are also used to further screen pixels.

@drlaw1558
Copy link
Collaborator Author

Looks good to me. One tiny comment on doc. The clean_showers module is surprising simple considering what it is trying to remove.

I was surprised at the large number for shower_x_stddev (18.0). Just for my understanding why is this set so large? The bad pixels have been naned out and the 3rd plane of the regions file is use to mark pixels that receive light from the sky and as I understand the shower_low and high reject are also used to further screen pixels.

Conceptually, it's large to enable the routine to reach across the masked-out IFU slices that can be 30+ pixels wide. Anecdotally, I tried a few different sizes and x=18 y=5 looked like it gave the best results visually for my test cases. It's entirely possible that there may be some better set of parameters though.

melanieclarke
melanieclarke approved these changes Feb 6, 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.

Everything looks good. Thanks for the contribution!

@melanieclarke melanieclarke merged commit 22e6984 into spacetelescope:main Feb 6, 2025
29 of 30 checks passed
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

@jemorrison jemorrison jemorrison approved these changes

Assignees
No one assigned
Labels
documentation straylight testing
Projects
None yet
Milestone
Build 11.3
Development

Successfully merging this pull request may close these issues.
3 participants
@drlaw1558 @melanieclarke @jemorrison
0