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

Jump performance #337

Merged
merged 12 commits into from
Feb 6, 2025
Merged

Conversation

t-brandt
Copy link
Contributor

@t-brandt t-brandt commented Feb 4, 2025

Resolves JP-3697

Closes #337

A number of changes to twopoint_difference.py and jump.py reduce memory usage by a factor of about 2 and improve runtime by a factor of anywhere from 3 to 20. Tested output files (noted below) are unchanged, except for MIRI shower detection in jw01701012001_02105_00001_mirifulong. This is due to a difference in replacing infilled saturated pixels after performing a convolution; the new version identifies fewer showers. An example comment on this is here:
https://jira.stsci.edu/browse/JP-3697?focusedId=760740&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-760740
Overflagging of jumps should no longer cause the pipeline to grind to a halt due to np.where combined with for loops over pixels; it should now have virtually no impact on performance.

Files tested, along with memory usage and runtimes in the jump step (all files are public except for program 5263):

File Old Runtime Old Memory New Runtime New Memory
jw01668002001_04101_00001_mirimage 68 s 27 GB 15 s 11 GB
jw01283001001_03101_00001_mirimage 420 s 70 GB 120s 30 GB
jw05263001001_05101_00001_nrs1 500 s 40 GB 30 s 23 GB
jw02079004001_03201_00001_nrca1 35 s 2 GB 6 s 2 GB
jw01701012001_02105_00001_mirifulong 13 s 0.4 GB 4 s 0.4 GB
jw01366001001_04101_00001-seg001_nis 155 s 40 GB 35 s 23 GB

A significant fraction of the line changes are due to fixing a mixture of 8-space indents and 4-space indents in twopoint_difference.py. Most of these lines were not actually changed.

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 February 4, 2025 18:24
@github-actions github-actions bot added the jump label Feb 4, 2025
Copy link

codecov bot commented Feb 4, 2025

Codecov Report

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

Project coverage is 86.14%. Comparing base (7342a4a) to head (68118a2).
Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
src/stcal/jump/twopoint_difference.py 87.96% 13 Missing ⚠️
src/stcal/jump/jump.py 98.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #337      +/-   ##
==========================================
+ Coverage   85.94%   86.14%   +0.20%     
==========================================
  Files          50       50              
  Lines        9401     9313      -88     
==========================================
- Hits         8080     8023      -57     
+ Misses       1321     1290      -31     

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

I'll review in more detail soon, but I have a couple initial comments below, then I can kick off regression tests.

src/stcal/jump/jump.py Outdated Show resolved Hide resolved
src/stcal/jump/twopoint_difference.py Outdated Show resolved Hide resolved
@drlaw1558
Copy link
Contributor

Adding a quick note that I've done some initial testing on this too, with good results. In the case of MIRI TSO observation jw03730013001_03101_00001-seg001_mirimage_uncal.fits the results are identical and jump step runtime goes down from 358 to 156 seconds.

src/stcal/jump/jump.py Outdated Show resolved Hide resolved
src/stcal/jump/jump.py Outdated Show resolved Hide resolved
src/stcal/jump/twopoint_difference.py Outdated Show resolved Hide resolved
src/stcal/jump/twopoint_difference.py Outdated Show resolved Hide resolved
src/stcal/jump/twopoint_difference.py Outdated Show resolved Hide resolved
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.

This looks great! I'd like an answer from someone on the one flagging logic question, though, and I have a few more questions below.

src/stcal/jump/jump.py Outdated Show resolved Hide resolved
src/stcal/jump/jump.py Outdated Show resolved Hide resolved
src/stcal/jump/jump.py Outdated Show resolved Hide resolved
src/stcal/jump/jump.py Show resolved Hide resolved
src/stcal/jump/twopoint_difference.py Show resolved Hide resolved
@melanieclarke
Copy link
Contributor

melanieclarke commented Feb 5, 2025

jwst regtests: https://github.com/spacetelescope/RegressionTests/actions/runs/13165362881
romancal regtests: https://github.com/spacetelescope/RegressionTests/actions/runs/13165381062

Romancal tests pass. JWST tests show expected differences only, matching main.

@melanieclarke
Copy link
Contributor

I think this should be good to go now. I will re-run the jwst tests one more time for the last small change, then merge.

@melanieclarke melanieclarke merged commit 651015d into spacetelescope:main Feb 6, 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.

5 participants