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-3793: Fix numbers of flagged groups #338

Merged
merged 9 commits into from
Feb 13, 2025

Conversation

drlaw1558
Copy link
Contributor

@drlaw1558 drlaw1558 commented Feb 7, 2025

Resolves JP-3793 regarding inaccurate calculation of the number of groups in the jump step. This PR updates prior #317 to account for recent unrelated changes.

EDIT: See also JP-3877. This PR ensures that long TSO exposures don't fall into the wrong processing type, reducing runtime from over an hour to about 1 minute in the example given there.

@drlaw1558 drlaw1558 requested a review from a team as a code owner February 7, 2025 16:09
@drlaw1558 drlaw1558 mentioned this pull request Feb 7, 2025
6 tasks
Copy link

codecov bot commented Feb 7, 2025

Codecov Report

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

Project coverage is 86.96%. Comparing base (9fba297) to head (f1278d7).
Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
src/stcal/jump/twopoint_difference.py 95.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #338      +/-   ##
==========================================
+ Coverage   86.78%   86.96%   +0.18%     
==========================================
  Files          57       57              
  Lines       10401    10271     -130     
==========================================
- Hits         9026     8932      -94     
+ Misses       1375     1339      -36     

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

Copy link
Contributor

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

Comparing to the previous PR, it looks like this is a clean copy of the original changes, also incorporating comments from Ken's review.

There are a couple errors in the unit tests that need cleaning up. When that's done, I can run regression tests.

tests/test_jump_twopoint_difference.py Outdated Show resolved Hide resolved
tests/test_jump_twopoint_difference.py Outdated Show resolved Hide resolved
total_noise_min_grps_fails = False

# Determine whether there are enough usable groups the two sigma clip options
if (only_use_ints and nints < minimum_sigclip_groups) \
Copy link
Collaborator

Choose a reason for hiding this comment

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

I recommend using parenthesis, instead of \ for multiple lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Swapped line formatting.

@melanieclarke
Copy link
Contributor

Romancal tests are okay, as expected - the failures are unrelated.

The jwst tests show some diffs for two of our regtest data sets, downstream of jump -- a NIRISS image and a NIRCam image. Since this is a bug fix, that's probably expected. In general, it looks like it found fewer jumps than before in both images. Are these diffs what you were expecting @drlaw1558?

@drlaw1558
Copy link
Contributor Author

@melanieclarke It's not surprising, but I'll take a look at the test cases just to make sure.

@drlaw1558
Copy link
Contributor Author

Ok, I’ve compared the rateint files for each of the two failing test cases. There are some number of pixels scattered throughout the images that vary visibly. It’s really low level though- some pixels improve, others get worse. On balance though, it looks like a significantly larger fraction of pixels improve with this PR than get worse (consistent with them now being processed through the appropriate branch of the jump step). I’d say that these differences are ok.

@melanieclarke
Copy link
Contributor

melanieclarke commented Feb 13, 2025

Thanks for checking the diffs @drlaw1558.

This all looks good to me. Romancal folks should not be impacted by jump step changes, but I'm tagging them for review for their information.

Edit: I reran the romancal regtests this morning and the unrelated failures are now cleaned up - all tests pass.

Copy link
Collaborator

@schlafly schlafly left a comment

Choose a reason for hiding this comment

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

Regression tests pass & we don't use the stcal jump step; approving.

@melanieclarke melanieclarke merged commit b58dae6 into spacetelescope:main Feb 13, 2025
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants