JP-3697: Jump Step Refactor #9039

kmacdonald-stsci · 2024-12-31T20:00:43Z

Closes #

This PR refactors the jump step using the refactored jump step interface in STCAL. Due to the simplified interface of the STCAL jump step jump.py and therun_detect_jumps function have been eliminated. All tests based on this function were consolidated into test_jump_step.py.

This PR requires the use of the STCAL PR spacetelescope/stcal#330

Tasks

news fragment change types...

changes/<PR#>.general.rst: infrastructure or miscellaneous change
changes/<PR#>.docs.rst
changes/<PR#>.stpipe.rst
changes/<PR#>.datamodels.rst
changes/<PR#>.scripts.rst
changes/<PR#>.fits_generator.rst
changes/<PR#>.set_telescope_pointing.rst
changes/<PR#>.pipeline.rst

stage 1

changes/<PR#>.group_scale.rst
changes/<PR#>.dq_init.rst
changes/<PR#>.emicorr.rst
changes/<PR#>.saturation.rst
changes/<PR#>.ipc.rst
changes/<PR#>.firstframe.rst
changes/<PR#>.lastframe.rst
changes/<PR#>.reset.rst
changes/<PR#>.superbias.rst
changes/<PR#>.refpix.rst
changes/<PR#>.linearity.rst
changes/<PR#>.rscd.rst
changes/<PR#>.persistence.rst
changes/<PR#>.dark_current.rst
changes/<PR#>.charge_migration.rst
changes/<PR#>.jump.rst
changes/<PR#>.clean_flicker_noise.rst
changes/<PR#>.ramp_fitting.rst
changes/<PR#>.gain_scale.rst

stage 2

changes/<PR#>.assign_wcs.rst
changes/<PR#>.badpix_selfcal.rst
changes/<PR#>.msaflagopen.rst
changes/<PR#>.nsclean.rst
changes/<PR#>.imprint.rst
changes/<PR#>.background.rst
changes/<PR#>.extract_2d.rst
changes/<PR#>.master_background.rst
changes/<PR#>.wavecorr.rst
changes/<PR#>.srctype.rst
changes/<PR#>.straylight.rst
changes/<PR#>.wfss_contam.rst
changes/<PR#>.flatfield.rst
changes/<PR#>.fringe.rst
changes/<PR#>.pathloss.rst
changes/<PR#>.barshadow.rst
changes/<PR#>.photom.rst
changes/<PR#>.pixel_replace.rst
changes/<PR#>.resample_spec.rst
changes/<PR#>.residual_fringe.rst
changes/<PR#>.cube_build.rst
changes/<PR#>.extract_1d.rst
changes/<PR#>.resample.rst

stage 3

changes/<PR#>.assign_mtwcs.rst
changes/<PR#>.mrs_imatch.rst
changes/<PR#>.tweakreg.rst
changes/<PR#>.skymatch.rst
changes/<PR#>.exp_to_source.rst
changes/<PR#>.outlier_detection.rst
changes/<PR#>.tso_photometry.rst
changes/<PR#>.stack_refs.rst
changes/<PR#>.align_refs.rst
changes/<PR#>.klip.rst
changes/<PR#>.spectral_leak.rst
changes/<PR#>.source_catalog.rst
changes/<PR#>.combine_1d.rst
changes/<PR#>.ami.rst

other

changes/<PR#>.wfs_combine.rst
changes/<PR#>.white_light.rst
changes/<PR#>.cube_skymatch.rst
changes/<PR#>.engdb_tools.rst
changes/<PR#>.guider_cds.rst

codecov · 2024-12-31T20:23:04Z

Codecov Report

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

Project coverage is 73.71%. Comparing base (d5bd5b8) to head (200717f).
Report is 881 commits behind head on main.

Files with missing lines	Patch %	Lines
jwst/jump/jump_step.py	93.22%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9039      +/-   ##
==========================================
- Coverage   73.73%   73.71%   -0.02%     
==========================================
  Files         373      372       -1     
  Lines       37277    37262      -15     
==========================================
- Hits        27486    27468      -18     
- Misses       9791     9794       +3

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

kmacdonald-stsci · 2025-01-06T14:07:11Z

Here is the regression test run that passed the jump tests.

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

melanieclarke

There are some style checks and unit tests that are failing in CI. Tests are probably just because it's pointing to the released version of stcal - can you point this to your branch instead for now so we can make sure tests are passing?

When that's fixed, I'd also like to run the full set of regression tests to make sure all the detector1 tests that depend on jump are passing.

melanieclarke · 2025-01-13T22:55:19Z

Testing locally, the test_exec_time_many_crs test in test_jump_step.py is failing, since it took 631s to complete: the limit in the test is 600s, with the baseline expectation of 100s (according to comments). The same test took 180s on main on my computer. Since this step is intended to gauge runtime for new jump code, I'm a little concerned. Have you benchmarked the new changes to make sure they're not making runtime worse?

t-brandt · 2025-01-14T02:18:39Z

After a quick check, I can confirm @melanieclarke's finding that test_exec_time_many_crs appears to be slower now: 16 seconds in this stcal+jwst version vs about 8 seconds for me in the main versions. That said, I am not worried about this test in particular. The many cosmic rays case is particularly unkind to the current approach of flagging neighbors with deeply nested for loops. This can be fixed, e.g., with
https://github.com/t-brandt/stcal/blob/412f078af633cea553c27b397340c463369b9e12/src/stcal/jump/twopoint_difference.py#L344
and subsequent lines. The neighbor flagging part of the code goes from 16 seconds in the new branch to just 40 ms with those changes.

t-brandt · 2025-01-14T03:20:45Z

jwst/jump/jump_step.py

+            if reffile_utils.ref_matches_sci(result, gain_m):
+                gain_2d = gain_m.data
+            else:
+                log.info('Extracting gain subarray to match science data')


Suggested change

log.info('Extracting gain subarray to match science data')

self.log.info('Extracting gain subarray to match science data')

Gives an error for me as is; breaks the pipeline.

t-brandt · 2025-01-14T03:21:05Z

jwst/jump/jump_step.py

+            if reffile_utils.ref_matches_sci(result, rnoise_m):
+                rnoise_2d = rnoise_m.data
+            else:
+                log.info('Extracting readnoise subarray to match science data')


Suggested change

log.info('Extracting readnoise subarray to match science data')

self.log.info('Extracting readnoise subarray to match science data')

Gives an error for me as is; breaks the pipeline.

t-brandt · 2025-01-14T03:50:28Z

Testing the new branches with a few reference files for benchmarking shows jump step runtimes that are mostly about the same as the current branches, and sometimes a little bit faster. Files tested:
jw01668002001_04101_00001_mirimage
jw01283001001_03101_00001_mirimage
jw02079004001_03201_00001_nrca1
jw01701012001_02105_00001_mirifulong
jw01366001001_04101_00001-seg001_nis
jw05263001001_05101_00001_nrs1 (proprietary)

melanieclarke · 2025-01-14T14:22:35Z

After a quick check, I can confirm @melanieclarke's finding that test_exec_time_many_crs appears to be slower now: 16 seconds in this stcal+jwst version vs about 8 seconds for me in the main versions. That said, I am not worried about this test in particular. The many cosmic rays case is particularly unkind to the current approach of flagging neighbors with deeply nested for loops. This can be fixed, e.g., with https://github.com/t-brandt/stcal/blob/412f078af633cea553c27b397340c463369b9e12/src/stcal/jump/twopoint_difference.py#L344 and subsequent lines. The neighbor flagging part of the code goes from 16 seconds in the new branch to just 40 ms with those changes.

Thanks for checking, Tim. It sounds like this is a performance improvement that can wait for the next PRs, then.

melanieclarke · 2025-01-14T18:06:11Z

Looks like unit tests are passing now (thanks!), but there's still a code style check that's failing for some unused variables.

Tim's comments above for the log references still need fixing as well.

When those are done, please run the full regtest suite.

t-brandt · 2025-01-15T16:12:10Z

The one last question/concern I have is that peak memory usage of the jump step seems to be slightly higher in this branch than in the previous branch. I haven't followed through on why that is. Testing on jw01668002001_04101_00001_mirimage

melanieclarke · 2025-01-30T16:40:48Z

@kmacdonald-stsci - to clean this PR up, please:

fix the conflict in the test_detect_jumps file
address style failures in the pre-commit check
address the documentation failure in the docs build
run full regression tests and post the link here

Adding JumpData class to running jump. Prepping for JumpData class. Updating import functions. Removing the dqflags parameter. The file jump.py is no longer needed, but there is a test suite that still uses the deprectaed run_detect_jumps function directly. Refactoring out run_detect_jumps in favor of using JumpData. Refactored jump step. Updating setting the parameters of the JumpData instance.

…The 'test_detect_jump.py' test suite needs to be integrated into 'test_jump_step.py' and the 'jump.py' needs to be deleted, since it is now unnecessary.

… moved to the main test suite.

…changes.

kmacdonald-stsci · 2025-02-03T19:52:49Z

The regression test failures and errors appear to be unrelated to this PR.

https://github.com/spacetelescope/RegressionTests/actions/runs/13119439663/job/36601746403

melanieclarke · 2025-02-03T19:55:51Z

Thanks @kmacdonald-stsci - those are expected failures for the current state on main. It looks like all unit tests are passing now, and regtests are clear, so I think we are ready to merge.

Withdrawing approval until stcal pr is merged.

tapastro

👍

kmacdonald-stsci requested a review from a team as a code owner December 31, 2024 20:00

github-actions bot added jump testing labels Dec 31, 2024

kmacdonald-stsci closed this Jan 3, 2025

kmacdonald-stsci reopened this Jan 6, 2025

kmacdonald-stsci requested review from t-brandt and melanieclarke January 6, 2025 14:11

melanieclarke reviewed Jan 13, 2025

View reviewed changes

t-brandt reviewed Jan 14, 2025

View reviewed changes

kmacdonald-stsci requested a review from a team as a code owner January 14, 2025 15:31

github-actions bot added the installation label Jan 14, 2025

melanieclarke mentioned this pull request Jan 23, 2025

JP-3697: Jump Step Refactor spacetelescope/stcal#330

Merged

7 tasks

kmacdonald-stsci force-pushed the jp_3697_jump branch from a72f779 to 2c45aa4 Compare January 30, 2025 16:47

kmacdonald-stsci added 8 commits February 3, 2025 11:45

Updating code to work properly with the JP-3677 update after rebase. …

54298ac

…The 'test_detect_jump.py' test suite needs to be integrated into 'test_jump_step.py' and the 'jump.py' needs to be deleted, since it is now unnecessary.

Moved all tests in test_detect_jumps.py to test_jump_step.py.

6690036

Removing unneeded import.

438ac4d

Removing unneeded comment.

a52dcc2

Removing unneeded module and corresponding test suite, which has been…

c8800e3

… moved to the main test suite.

Removing unneeded code, adding some comments, and making minor style …

368e5f7

…changes.

Adding fragment for change log.

dd14c68

kmacdonald-stsci added 7 commits February 3, 2025 11:45

Pointing to necessary PR branch.

e967ff4

Updating ruff style checks.

f8de3b2

Updating the call to the log.

0c1b3de

Updating the tests based on ruff checks.

2e1e9e9

Correcting misspelling.

2c596a9

Removing white space.

82511e5

Updating docstrings.

7184976

kmacdonald-stsci force-pushed the jp_3697_jump branch from 8ebdb29 to 7184976 Compare February 3, 2025 16:46

Merge branch 'main' into jp_3697_jump

200717f

melanieclarke previously approved these changes Feb 3, 2025

View reviewed changes

tapastro requested review from melanieclarke and tapastro February 3, 2025 20:08

tapastro approved these changes Feb 3, 2025

View reviewed changes

tapastro merged commit 537106e into spacetelescope:main Feb 3, 2025
29 checks passed

melanieclarke mentioned this pull request Feb 3, 2025

Point stcal to main #9132

Closed

10 tasks

stscijgbot-jp mentioned this pull request Feb 4, 2025

Jump step refactoring #8679

Closed

kmacdonald-stsci deleted the jp_3697_jump branch February 5, 2025 17:26

	log.info('Extracting gain subarray to match science data')
	self.log.info('Extracting gain subarray to match science data')

	log.info('Extracting readnoise subarray to match science data')
	self.log.info('Extracting readnoise subarray to match science data')

JP-3697: Jump Step Refactor #9039

JP-3697: Jump Step Refactor #9039

Uh oh!

Conversation

kmacdonald-stsci commented Dec 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tasks

stage 1

stage 2

stage 3

other

Uh oh!

codecov bot commented Dec 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kmacdonald-stsci commented Jan 6, 2025

Uh oh!

melanieclarke left a comment

Choose a reason for hiding this comment

Uh oh!

melanieclarke commented Jan 13, 2025

Uh oh!

t-brandt commented Jan 14, 2025

Uh oh!

t-brandt Jan 14, 2025

Choose a reason for hiding this comment

Uh oh!

t-brandt Jan 14, 2025

Choose a reason for hiding this comment

Uh oh!

t-brandt commented Jan 14, 2025

Uh oh!

melanieclarke commented Jan 14, 2025

Uh oh!

melanieclarke commented Jan 14, 2025

Uh oh!

t-brandt commented Jan 15, 2025

Uh oh!

melanieclarke commented Jan 30, 2025

Uh oh!

kmacdonald-stsci commented Feb 3, 2025

Uh oh!

melanieclarke commented Feb 3, 2025

Uh oh!

tapastro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kmacdonald-stsci commented Dec 31, 2024 •

edited

Loading

codecov bot commented Dec 31, 2024 •

edited

Loading