-
Notifications
You must be signed in to change notification settings - Fork 171
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
CHARGELOSS should be propagated to all subsequent groups in a ramp. #8331
Comments
Comment by Howard Bushouse on JIRA: Fixed by #8336 |
Comment by Paul Goudfrooij on JIRA: This sounds strange. Can someone point out a dataset and pixel position where the accumulated signal in group N is lower than that in group N-1? |
Comment by Rachel Plesha on JIRA: Kenneth MacDonald can you point us to the examples in the regression test where this was an issue so that we can test the data with the latest build? |
Comment by Kenneth MacDonald on JIRA: In my analysis of the C extension for ramp fitting in the regression test, from the JWST root directory, I ran pytest --bigdata jwst/regrest -k rate For each of these tests, I modified the tests so that the results of the jump step always got saved. In the jump file: jw01094001002_02107_00001_nis_jump.fits At (row, column) = (1162, 1628), zero based, the ramp for integration 2 at group 6 (computing both zero based), is flagged as 129, which is CHARGELOSS and DO_NOT_USE. No other group is flagged like this, so it is an example of a ramp where for whatever reason, only one group in the middle of a ramp got flagged as CHARGELOSS. |
Comment by Paul Goudfrooij on JIRA: Hi Kenneth MacDonald, I just downloaded that dataset and ran it though detector1 with save_calibrated_ramp = True, and I do not replicate what you are saying, in that all groups after group 6 also get flag = 129, as intended. So I figure this can be closed out now.
|
Comment by Kenneth MacDonald on JIRA: Paul Goudfrooij which version of the code are you running? In March, I updated the code to fix this bug: #8336 |
Comment by Paul Goudfrooij on JIRA:
|
Comment by Kenneth MacDonald on JIRA: That version includes the bug fix, so you won't be able to reproduce the bug with that version. |
Comment by Paul Goudfrooij on JIRA: Kenneth MacDonald - Understood, this was to test whether it indeed was fixed. I will try to reproduce the bug separately. EDIT: actually, looking at the [SCI] array at that position, I don't understand why group 6 got the CHARGELOSS flag in the first place:
The thing is that the group-6 value of 1700.57 is not above the minimum threshold to receive the CHARGELOSS flag - that threshold is 23934.0 for the filter used in this exposure. So, the fact that this group got that flag is an issue by itself. |
Comment by Kenneth MacDonald on JIRA: The logic for the flagging is done here:
|
Comment by Paul Goudfrooij on JIRA: Thanks for that link. I now see why that group was assigned the CHARGELOSS flag. It is because that group in a neighboring pixel got the CHARGELOSS flag due to "data > signal_threshold". So.. problem solved. Thanks for prompting us about it. |
Issue JP-3560 was created on JIRA by Kenneth MacDonald:
When the CHARGELOSS flag is set in an integration ramp, all subsequent groups in the ramp should be flagged as CHARGELOSS as well. For example, if in an m_ long ramp, group _n is flagged as CHARGELOSS, then all groups {}n{}{}+k{} should be flagged as CHARGELOSS as well, where n+k < m and k >= 0.
Currently, the CHARGELOSS flag is set only for groups meeting the flagging threshold based on the assumption that all later will meet the threshold to be flagged as CHARGELOSS. Regression testing has found examples that this not be true, so the charge migration step should force all subsequent groups be flagged whenever a single group in a ramp is flagged.
The text was updated successfully, but these errors were encountered: