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

Reference pixels set to DO_NOT_USE in DQ is fragile #7015

Open
jdavies-st opened this issue Sep 2, 2022 · 6 comments · May be fixed by #7017
Open

Reference pixels set to DO_NOT_USE in DQ is fragile #7015

jdavies-st opened this issue Sep 2, 2022 · 6 comments · May be fixed by #7017

Comments

@jdavies-st
Copy link
Collaborator

Currently, the way that reference pixels get set to DO_NOT_USE in calwebb_detector1 is by relying on them being set to zero in the GAIN reference file. If they are set to zero there (and there's not a strong reason they should be), then ramp_fit flags any gain values <0 or NaN as NO_GAIN_VALUE and DO_NOT_USE here

https://github.com/spacetelescope/stcal/blob/8e00460560eea757f6959ef17b5877583bc60de8/src/stcal/ramp_fitting/utils.py#L981-L1017

If they are not set to zero in the GAIN reference file, as was the case briefly with NIRCam gain reference files this week, then the reference pixels never get set to DO_NOT_USE, ramp_fitting fits them, and they end up in the output _i2d mosaic produced by calwebb_image3, producing unwanted artifacts. See right side below.

Screen Shot 2022-09-01 at 12 00 15

Setting GAIN reffile ref pix to zero so that they get flagged as DO_NOT_USE in ramp_fitting is a fragile and roundabout way to get that job done.

@hbushouse is there a good reason to not set the reference pixels DQ values to DO_NOT_USE at the end of the refpix step? They've been corrected and now can be ignored from here on out.

@hbushouse
Copy link
Collaborator

This is another one of those little details who's reasons are lost to the mists of time (at least it's pretty misty in my mind). I recall at least one Cal WG meeting in which the question was asked whether we should bother doing ramp fitting to reference pixels and the answer was along the lines of "Sure, why not; you never know what kind of interesting info that may turn up. The slopes don't get used for anything later anyway, so what can it hurt?" So they were not flagged and left as "normal" pixels.

And then of course some years later it was discovered that they were not getting removed when doing mosaics and I'm sure we implemented some kind of fix back then (when it was discovered the first time). Will have to dig around to see what the method was back then. Perhaps that's when we added the "REFERENCE PIXEL" DQ flag label and assumed that steps like resample would reject based on that flag alone (i.e. not also requiring DO_NOT_USE). Will need to go back and try to find the history of this. I'm 99% sure we supposedly fixed this already.

@nden
Copy link
Collaborator

nden commented Sep 2, 2022

That's my recollection too. It would be good to start a kind of algorithms subsection to each step in RTD to document things like this so the knowledge is not lost.

@stscirij
Copy link
Contributor

stscirij commented Sep 2, 2022

I vaguely remember the addition of the REFERENCE_PIXELS DQ bit was to make the reference pixel processing more straightforward, but it could certainly be used to exclude regions from subsequent processing. Perhaps the NON_SCIENCE DQ bit would be more appropriate to set after the refpix calculations are done?

@hbushouse
Copy link
Collaborator

But, but, but ... here in the resample step it sure looks to me like pixels flagged with REFERENCE_PIXEL are set to be rejected: https://github.com/spacetelescope/jwst/blob/master/jwst/resample/resample.py#L284

Is this somehow not working as intended?

@hbushouse
Copy link
Collaborator

But at the same time the top level resample_step module has https://github.com/spacetelescope/jwst/blob/master/jwst/resample/resample_step.py#L20 which does not pay attention to REFERENCE_PIXEL and gets stored in the kwargs passed around to lower-level routines: https://github.com/spacetelescope/jwst/blob/master/jwst/resample/resample_step.py#L212

So perhaps what we have here is failure to communicate between the various levels of the resample step?

@jdavies-st
Copy link
Collaborator Author

jdavies-st commented Sep 5, 2022

I think the thing to remember is that DO_NOT_USE needs to be set if the pixels are to not be used in subsequent (or the current) step, previous hacks aside. All the other higher bits are informational only. So we can set many bits, but in the end, resample should be using all the bits except DO_NOT_USE. This is the design architecture at least. The question then becomes which is the best place to set the reference pixels to DO_NOT_USE. At the end of refpix? Or later? ramp_fitting? Later still?

Of course resample has special problems because the code was pulled from drizzlepac which has a very different bit usage architecture, and it's never fully transitioned to the correct jwst one. I.e. the whole goodbits architecture. It should be jettisoned to be consistent with the rest of the pipeline. There used to be an open issue on this which I cannot find that I know @stscieisenhamer worked on.

The behavior up until this past week for the pipeline for NIRCam was that the GAIN reffile had no gain value in the reference pixels, and therefore there was no ramp fit in those areas - they were set to zero and flagged as DO_NOT_USE and NO_GAIN_VALUE. The newest GAIN reffiles that were delivered Friday, the current bestrefs, also have this feature. But it is fragile. Hence the current issue. And other instruments, such as MIRI do not set reference pixels in the GAIN reffile to zero. So for those, reference pixels will show up in the output mosaics.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants