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-2693: Flag groups do_not_use after detected ramp jumps #110

Merged
merged 16 commits into from Aug 17, 2022

Conversation

karllark
Copy link
Contributor

@karllark karllark commented Jul 26, 2022

Resolves JP-2693

This PR implements the enhancement to the jump step where groups after detected ramps are flagged as jumps. This "corrects" transients seen that make the slope after a jump different than before. This PR allows for flagging to be different for two different jump thresholds.

Checklist

  • added entry in CHANGES.rst (either in Bug Fixes or Changes to API)
  • updated relevant tests
  • updated relevant documentation
  • updated relevant milestone(s)
  • added relevant label(s)

@codecov
Copy link

codecov bot commented Aug 10, 2022

Codecov Report

Merging #110 (493d803) into main (8e00460) will increase coverage by 0.11%.
The diff coverage is 90.00%.

@@            Coverage Diff             @@
##             main     #110      +/-   ##
==========================================
+ Coverage   72.11%   72.23%   +0.11%     
==========================================
  Files          16       16              
  Lines        2510     2528      +18     
==========================================
+ Hits         1810     1826      +16     
- Misses        700      702       +2     
Impacted Files Coverage Δ
src/stcal/jump/jump.py 12.32% <0.00%> (-0.35%) ⬇️
src/stcal/jump/twopoint_difference.py 80.29% <100.00%> (+2.60%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@karllark
Copy link
Contributor Author

Not sure how to update the docs for this PR. There doesn't seem to much in the way of docs at all. Please advise.

@hbushouse
Copy link
Collaborator

Not sure how to update the docs for this PR. There doesn't seem to much in the way of docs at all. Please advise.

If you've at least checked to make sure that no updates are needed, that's good enough. Just leave the box unchecked.

@karllark karllark marked this pull request as ready for review August 10, 2022 17:15
@karllark
Copy link
Contributor Author

Ready for review. Likely, the corresponding PR in jwst should be reviewed at the same time.

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

@cshanahan1 cshanahan1 left a comment

Choose a reason for hiding this comment

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

still looking at this and testing it out but i left a few comments

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

@hbushouse hbushouse left a comment

Choose a reason for hiding this comment

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

Looks like all review comments have been addressed. @cshanahan1 are you good with this now?

@hbushouse
Copy link
Collaborator

Need approval from someone from Roman. @ddavis-stsci ?

Copy link
Collaborator

@ddavis-stsci ddavis-stsci left a comment

Choose a reason for hiding this comment

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

The roman regression tests pass with that PR (and pinning jsonschema==4.9.0) so I think we're good to go.
The regression test is at: https://plwishmaster.stsci.edu:8081/me/my-views/view/all/job/RT/job/romancal/607/

@hbushouse
Copy link
Collaborator

OK, it sounds like everyone is on-board with this PR and hence I'm merging it now.

@hbushouse hbushouse merged commit 73a88c5 into spacetelescope:main Aug 17, 2022
@hbushouse hbushouse added this to the 1.1.0 milestone Aug 19, 2022
@jdavies-st
Copy link
Collaborator

I'm trying to understand this PR and the behavior of JumpStep here. This allows flagging a pixel in a subsequent group after the initial jump is detected in that pixel.

The main need for this is because in many readout patterns, groups are actually averages of many frames, and the jump occurs within the group, not between groups.

If the jump happens within the group (i.e. between frames in a group that are then averaged), then it corrupts two differences between groups. This also makes them harder to detect because they can have half the amplitude.

Given the 2-point difference method, is there a reason to believe one should flag the group before or after? Is it more sensitive to the first or the second difference corrupted by the jump within the group? If we can't tell, wouldn't it be best to throw out both, i.e. the group before and after the detected jump? Or is the current model more likely to find the first difference affected, and therefore it's sufficient to flag only the subsequent group?

So given the above, generally, if model.meta.exposure.nframes > 1, i.e. there are averaged frames in a group, then one should always turn on after jump flagging with a very low DN threshold?

The other driver for this I suspect is persistence or leakage of charge from saturated pixels in a jump into unsaturated areas in subsequent reads? One sees this easily around the saturated cores of large events (snowballs) in the NIR detectors.

Just trying to understand these parameters so I can tune them to the problems in the data I'm seeing.

@karllark
Copy link
Contributor Author

karllark commented Aug 9, 2023

The main need for this is because in many readout patterns, groups are actually averages of many frames, and the jump occurs within the group, not between groups.

No, this is not the motivation for this PR. The motivation is stated correctly, there is a transient seen after ramp jumps that lasts for multiple groups. This is seen when there is only one frame/read in a group. This has to do with the how the pixel is reacting the the delta function injection of charge. Completely independent of the number of frames is a group. Writing a report showing the transients is on my todo list.

There is an issue with dealing with ramp jumps that happen in groups with multiple frames. That is something that is not solved explicitly, but is partially solved as the 2pt diff algorithm will find two 2pt diffs that should be flagged as jumps - at least for the larger CRs.

The issue of charge spilling from saturated groups is handled in the saturation step where the saturation flag is expanded to the 4 neighboring pixels.

@karllark
Copy link
Contributor Author

karllark commented Aug 9, 2023

Also note that the transient is seen for multiple groups after a ramp jump, so not just one. For the really large jumps, the transient may go on for quite a few groups.

@jdavies-st
Copy link
Collaborator

Thanks Karl. That helps. I will will fiddle with the JumpStep knobs some more then and see what works. 👍

@hbushouse
Copy link
Collaborator

@karllark Remind me: is this "transient" after a CR hit only seen in the mid-IR detectors, or near-IR as well?

@karllark
Copy link
Contributor Author

karllark commented Aug 9, 2023

That analysis I did was for the MIR detectors. Discussions with others (e.g., Karl Misselt) indicated that such transients are also seen on the NIR detectors, especially for the large CRs.

@jdavies-st
Copy link
Collaborator

jdavies-st commented Aug 25, 2023

@karllark, is the transient charge seen in subsequent groups after a CR hit different from general persistence due to high signal in MIRI? I.e. is the delta function charge at N counts different from N counts due to a bright star the build linearly-ish? I suspect so, but also wonder if this after_jump flagging is really flagging persistence.

On the NIR detectors, it is clearly persistence from saturated (or near saturated) pixels in previous groups that then persist for subsequent groups and often subsequent exposures up 2000 to 3000 seconds later. It doesn't seem to matter whether it's a core of a bright star or a core of a snowball CR. It persists.

For the NIR case, it seems more sensible to take care of this in the persistence step by either masking or modeling it, hopefully exposure-to-exposure at some point via level3 processing.

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.

None yet

6 participants