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

Saturation speedup #331

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

t-brandt
Copy link

@t-brandt t-brandt commented Jan 3, 2025

Resolves JP-3820

Closes #

This PR improves the performance of the saturation step, reducing its runtime by as much as a factor of 5-10 while leaving results unchanged. It does this primarily with three changes. First, when a pixel saturates, that saturation is propagated into all subsequent reads using a running boolean tally of pixels that have saturated. Second, ndimage.binary_dilation is very slow, an order of magnitude slower than writing the operations out explicitly. Finally, if there is no averaging in groups, there is no point in the diluted saturation check; this check is skipped if the dilution factor is 1.

Tasks

  • update or add relevant tests
  • update relevant docstrings and / or docs/ page
  • Does this PR change any API used downstream? (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)
    • run regression tests with this branch installed ("git+https://github.com/<fork>/stcal@<branch>")
news fragment change types...
  • changes/<PR#>.apichange.rst: change to public API
  • changes/<PR#>.bugfix.rst: fixes an issue
  • changes/<PR#>.general.rst: infrastructure or miscellaneous change

@t-brandt t-brandt requested a review from a team as a code owner January 3, 2025 03:44
Copy link

codecov bot commented Jan 3, 2025

Codecov Report

Attention: Patch coverage is 92.68293% with 3 lines in your changes missing coverage. Please review.

Project coverage is 86.91%. Comparing base (35726e9) to head (d688267).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
src/stcal/saturation/saturation.py 92.68% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #331       +/-   ##
===========================================
+ Coverage   29.57%   86.91%   +57.33%     
===========================================
  Files          36       49       +13     
  Lines        7949     8972     +1023     
===========================================
+ Hits         2351     7798     +5447     
+ Misses       5598     1174     -4424     

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

# We want to flag saturation in all subsequent groups after
# the one in which it was found. Use this boolean array to
# keep a running tally of pixels that have saturated.
previously_saturated = np.zeros(data[0, 0].shape, dtype='bool')
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's clearer to use shape=(row,col), rather than shape=data[0,0].shape.

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good; I changed it.

@drlaw1558
Copy link
Contributor

drlaw1558 commented Jan 3, 2025

Just tested this empirically as well. Can confirm that the groupdq array is unaffected both for an ungrouped case (jw01864005001_03102_00001_mirimage), a grouped IRS2 case (jw01908004001_02101_00005_nrs1, used for previous testing of spacetelescope/jwst#8593), and a grouped NIRCam case (jw02756003001_03101_00001_nrcalong), with appreciable runtime gains in the long-integration limit (17 seconds to 3 seconds in the 292 group MIRI case here).

Also looks like the slow binary_dilation behavior has been a known issue for the last few years: scipy/scipy#13991

changes/331.general.rst Outdated Show resolved Hide resolved
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