JP-4000: Persistence Options Added

[kmacdonald-stsci]

Resolves JP-4000

This PR addresses a rough draft of how to handle persistence options to the persistence step.

Tasks

• If you have a specific reviewer in mind, tag them.
• add a build milestone, i.e. Build 12.0 (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 changelog readme for instructions)
  • 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

[codecov]

Codecov Report

❌ Patch coverage is 93.82716% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.85%. Comparing base (9136ddc) to head (0a5410d).

Files with missing lines	Patch %	Lines
jwst/persistence/persistence.py	92.50%	3 Missing ⚠️
jwst/persistence/persistence_step.py	95.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10091      +/-   ##
==========================================
- Coverage   86.86%   86.85%   -0.01%     
==========================================
  Files         375      375              
  Lines       40346    40069     -277     
==========================================
- Hits        35047    34803     -244     
+ Misses       5299     5266      -33     

☔ View full report in Codecov by Harness.
📢 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.

[drlaw1558]

I've been trying to test this out and I'm not sure I'm using it correctly. Let's say I have two exposures, jw05014008002_02107_00001_mirimage_uncal.fits (exp 1) and jw05014008003_02107_00001_mirimage_uncal.fits (exp 2)

I want to run the persistence step to ensure that things that saturate in exp1 get flagged as PERSISTENCE in exp2.

I run
strun calwebb_detector1 jw05014008002_02107_00001_mirimage_uncal.fits --steps.persistence.save_persistence=True --steps.persistence.persistence_time=8000

which creates an asdf file that should describe pixel timings.

Then I process the second exposure with
strun calwebb_detector1 jw05014008003_02107_00001_mirimage_uncal.fits --steps.persistence.persistence_array_file='theasdffile.asdf'

but it looks like the DQ array of the rate file isn't flagging the persistence anywhere. If I also provide --steps.persistence.persistence_time=8000 in the call to the second exposure I still don't get DQ flags, but I do get a big block of NaN-valued SCI array pixels in the relevant areas?

[melanieclarke]

In addition to addressing David's comments from testing, this needs some preliminary clean up on the software side before we continue reviews:

1. pre-commit, changelog, and docs tests are all failing.
2. unit tests are failing (some notes below on how to fix)
3. the regression test runs only a trivial use case. We'll need regression tests that demonstrate both: initial persistence flagging and saving the asdf file, and using an existing asdf file to flag persistence in a subsequent exposure.

[melanieclarke]

I think this should default to True, since it requires a non-default persistence_time to do anything useful.

[melanieclarke]

Unit tests are failing because some steps run detector1 on synthetic data that doesn't include values assumed to be present inside do_all. Since this function does nothing if persistence_time is None, I think it would be helpful to check for that input and just skip the step instead of calling do_all (in addition to defaulting to skip=True).

[drlaw1558]

Adding a couple other quick comments:

• Oftentimes it will be necessary to pass persistence information between multiple exposures. In the example I'm working with for instance, persistence in program 5014 affects later exposure in 5014 and also in the next program PID 5407. Does the save_persistence infrastructure pass along input persistence information from the prior exposure (input via the asdf file) in the output asdf file, or just from the exposure being processed? It seems like it might be easiest to have a single reference asdf file that gets added to each time a new exposure is processed rather than many such files? There's the risk of collision of course, but since the file would be an agglomeration of info from many exposures anyway perhaps that doesn't matter, allowing the filename to be more predictable.
• It may be worth default to some finite-valued persistence_time instead of None. Users can always customize it based on their needs, but a starting point might be informative so long as the results are only affecting the DQ plane. This would also deal with the issue that the step might run, and report completion, but be doing nothing if this keyword were not set. Just as a stand-in for now I'd say perhaps 900 seconds, and instruments can always customize further with param ref files.

[kmacdonald-stsci]

Adding a couple other quick comments:

• Oftentimes it will be necessary to pass persistence information between multiple exposures. In the example I'm working with for instance, persistence in program 5014 affects later exposure in 5014 and also in the next program PID 5407. Does the save_persistence infrastructure pass along input persistence information from the prior exposure (input via the asdf file) in the output asdf file, or just from the exposure being processed? It seems like it might be easiest to have a single reference asdf file that gets added to each time a new exposure is processed rather than many such files? There's the risk of collision of course, but since the file would be an agglomeration of info from many exposures anyway perhaps that doesn't matter, allowing the filename to be more predictable.
• It may be worth default to some finite-valued persistence_time instead of None. Users can always customize it based on their needs, but a starting point might be informative so long as the results are only affecting the DQ plane. This would also deal with the issue that the step might run, and report completion, but be doing nothing if this keyword were not set. Just as a stand-in for now I'd say perhaps 900 seconds, and instruments can always customize further with param ref files.

"Does the save_persistence infrastructure pass along input persistence information from the prior exposure (input via the asdf file) in the output asdf file, or just from the exposure being processed?"

The ASDF file written out if save_persistence is True will output the persistence_array.

https://github.com/kmacdonald-stsci/jwst/blob/57c3519b88325cf2ceabcba1d41273d1a494bff3/jwst/persistence/persistence_step.py#L127

If no persistence array file is inputed, it will created one to be used during persistence.

https://github.com/kmacdonald-stsci/jwst/blob/57c3519b88325cf2ceabcba1d41273d1a494bff3/jwst/persistence/persistence_step.py#L101

The information in this array is end time, in epoch time, of any active persistence flagging window. A value of 0.0 indicates no active window. The end time of the windows are calculated as follows:

https://github.com/kmacdonald-stsci/jwst/blob/57c3519b88325cf2ceabcba1d41273d1a494bff3/jwst/persistence/persistence.py#L147

1. The current time of the current group is calculated.
2. A pixel with a non-zero entry in the persistence array that is less than the current group time, it is set to 0.0 in the persistence array, since that window closed.
3. A pixel with the current group marked as the first saturated group will have a new end time for the flagging window computed (current time plus window length) and entered into the persistence array.
4. Any pixel with a non-zero entry that is greater than the current group time will have the group DQ array flagged as PERSISTENCE (and DO_NOT_USE if selected).

The current persistence array, within an exposure, persists across integrations. If a persistence array is desired to persist across exposures, it must be saved to an ASDF file that will then be used as an input for the next exposure. This must be done manually.

The file names have a time stamp associated with them to allow developers using this feature to change parameters without clobbering previous computations. For example, a user may want to investigate various length persistence windows. Or use a previously computed persistence array as input for more than one exposure, without worry the file will be clobbered with each run of the persistence step.

I thought the current naming system was a good idea, but if a different naming convention is desired, with maybe the window length, rather than datetime as part of the suffix or no additional uniqueness to he suffix, that's fine.

"It may be worth default to some finite-valued persistence_time instead of None."

The input parameters can be changed. I had originally set this to be None when I thought this feature would be added to the existing processes, rather than simply replacing it. I set it to None as default because that would indicate skipping the persistence flagging based on the newly created timing window, but allowing for the other processes to be run if the step isn't skipped. The spec can be updated and the control flow correspondingly updated.

[tapastro]

Following David's question, I tested the PR on two exposures from one dataset. I parametrized the step with

strun calwebb_detector1 jw02772101001_02101_00001_nrcalong_uncal.fits --steps.persistence.persistence_time=50000 --steps.persistence.save_persistence=True --steps.persistence.skip=False

That run generated a persistence file titled jw02772101001_02101_00001_nrcalong_pers20260318113812109525.asdf. Using that as input to the next exposure's persistence call:

strun calwebb_detector1 jw02772101001_02101_00002_nrcalong_uncal.fits --steps.persistence.persistence_time=50000 --steps.persistence.save_persistence=True --steps.persistence.skip=False --steps.persistence.persistence_array='jw02772101001_02101_00001_nrcalong_pers20260318113812109525.asdf'

The output persistence file is then titled jw02772101001_02101_00001_nrcalong_pers20260318113812109525_pers20260318122231819751.asdf. That should be fixed to not concatenate the pers-timestamp, but to replace it.

Looking at the differences between the outputs, I don't see any difference between the two first exposures (the first call above compared to the equivalent call with no persistence). But for the second exposure, I see differences in the SCI array. It looks like partially saturated pixels are misbehaving - the attached plot shows a plot of the difference image (jw02772101001_02101_00002_nrcalong_rate-withpersistence - {...}-nopersistence) - it seems as though the the persistence step is causing ramp_fit to report smaller rates for the pixels on the edge of saturated sources, likely partially saturated.

[drlaw1558]

Thanks @kmacdonald-stsci and @tapastro ; sounds like there's more to understand in terms of how this step can interact with later steps and might need some iteration.

[tapastro]

The output persistence file is then titled jw02772101001_02101_00001_nrcalong_pers20260318113812109525_pers20260318122231819751.asdf. That should be fixed to not concatenate the pers-timestamp, but to replace it.

This issue is still present - we need some filename cleaning of the input persistence array filename if it is also requested that the step save a fresh version. I'd also suggest that we truncate the timestamp by ~3 digits - we probably don't need to go beyond milliseconds.

[tapastro]

Thanks for your efforts, Ken! I think we're good to go.

[tapastro]

Scratch that - I'll get the stcal PR in, then we'll need to update the pyproject here to point back to stcal/main.

I'm also going to get one more set of RTs going, just to have it in hand.

RTs here, with both PRs: https://github.com/spacetelescope/RegressionTests/actions/runs/27361455924

[tapastro]

RT run shows errors caused by existing regtests that use parameters which no longer exist: see test_niriss_image and test_nircam_persistence. Those tests will need to be updated to be compliant with the new step spec before we can merge.