JP-1694: fix outlier detection #5601
Conversation
jemorrison
requested review from
stsci-hack,
jdavies-st and
hbushouse
and removed request for
stsci-hack
Codecov Report
@@ Coverage Diff @@
## master #5601 +/- ##
==========================================
- Coverage 74.47% 74.16% -0.32%
==========================================
Files 416 416
Lines 38107 38617 +510
Branches 5157 5157
==========================================
+ Hits 28379 28639 +260
- Misses 9728 9978 +250
*This pull request uses carry forward flags. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
I assume that the computed background needs to be added back into the blotted image because the individual cal image that it's going to be compared to still has the background signal in it, right? If so, that makes sense and hence that change seems appropriate.
I don't, however, see any change here related to the proper setting of good_bits.
|
I put the change for the good_bits in the cfg file I was using. I am not sure how other instruments handle NON_SCIENCE. So I don't k now if that is a global default change or something we put in the code or something that gets added to the pars-outlierdetection ? I am still trying to understand how the 'pars' method will work. |
|
We recently stripped out all of the default parameter entries from all .cfg files for pipeline and steps, so now the defaults are set only in each step's Theoretically, however, the approach we have taken in most, if not all, pipeline steps with regard to which DQ values to use to signify that a pixel is to be ignored is in fact just the |
jdavies-st
changed the title
|
@hbushouse on the reason the background has to be added - yes the cal image still has the background signal in it. @cracraft @mwregan2 @karllark Is there any reason why the pixels in the lower 3 masks are set to NON_SCIENCE and not NON_SCIENCE+DO_NOT_USE for the cal image pipelines ? |
|
Going back through emails with Ors, I found this note about the masking: Yes, the 4QPM should not be used. I did not mask the 4QPMs for 2 reasons. 1.) for onboard flats I needed them. 2.) Could be useful for image registration or could contain information that is valuable. Although I should mark NON_SCIENCE, to remove them from the mosaics, I totally agree. Is NO_FLAT_FIELD needed too? We also talked about which mask values to use (though didn't discuss DO_NOT_USE) in JP-164. |
|
I can not find where MIRI Imager pixels are marked as NON_SCIENCE in the pipeline. Could someone point me ? I did a grep on NON_SCIENCE in the pipeline that pointed me to photom step. But when I look at the code it seems NON_SCIENCE is set for NIRSPec data but I can not find where it is set for MIRI imager data. |
|
NON_SCIENCE DQ values are in the flat files. It's not set as part of the pipeline, just read in from reference files. |
|
@cracraft so would it cause down stream problems if as part of the flat field step we set pixels with NON_SCIENCE to NON_SCIENCE + DO_NOT_USE. You mentioned above that on board flats need these regions. Is that for making the flat fields not after they are applied. |
|
Ors would have to answer why he wanted to use the flags he did, and what effect he anticipated it having through the pipeline. I don't know if that would be a problem or not. I don't see that it would, but others should weigh in on that. |
|
I think it is fine to change the default in |
That's a solution that achieves the proper results, but goes against the philosophy of having all steps only reject pixels flagged with |
|
Agree Howard, but as I mentioned in a previous thread on this topic, NON_SCIENCE might be DO_NOT_USE for one step, but fine to use on another step. I.e. if two steps that have different requirements, In the end, this DQ flag is being propagated from the flat reference file. That doesn't seem ideal in controlling what happens in |
|
@hbushouse |
jemorrison
force-pushed
the
outlier_detection_JP_1694
branch
from
4648868 to
01ed074
Compare
| @@ -421,7 +421,8 @@ def flag_cr(sci_image, blot_image, **pars): | |||
| # | |||
| # Model the noise and create a CR mask | |||
| diff_noise = np.abs(sci_data - blot_data) | |||
| ta = np.sqrt(np.abs(blot_data + subtracted_background) + err_data ** 2) | |||
| #ta = np.sqrt(np.abs(blot_data + subtracted_background) + err_data ** 2) | |||
There was a problem hiding this comment.
Please delete the commented-out line.
| @@ -406,7 +406,7 @@ def flag_cr(sci_image, blot_image, **pars): | |||
| exptime = sci_image.meta.exposure.exposure_time | |||
|
|
|||
| sci_data = sci_image.data * exptime | |||
| blot_data = blot_image.data * exptime | |||
| blot_data = (blot_image.data +subtracted_background) * exptime | |||
There was a problem hiding this comment.
Get a space after that + operator.
| # Find all pixels in the flat that have a DQ value of NON_SCIENCE | ||
| # add the DO_NOT_USE flag to these pixels. We don't want to use these pixels | ||
| # in futher steps | ||
| flag_nonsci = np.bitwise_and(science.dq, dqflags.pixel['NON_SCIENCE']).astype(np.bool) | ||
| science.dq[flag_nonsci] = np.bitwise_or(science.dq[flag_nonsci], dqflags.pixel['DO_NOT_USE']) |
There was a problem hiding this comment.
Adding NON_SCIENCE doesn't seem to have any effect on the issue described in the ticket. I.e. in the dataset from the ticket, running calwebb_image3 starting with the _cal.fits files as they are resolves the issue with just the changes in outlier_detection, without having to rerun flat_field on the input data.
So what is the purpose of this section of code? Currently the NON_SCIENCE area in MIRI data gets set to zero in the resampled _i2d.fits output. I.e., I don't see a problem with what is happening currently.
|
The preferred method is for outlier detection to have good_bits = DO_NOT_USE only. If we do not go back to the flat field step and flag NON_SCIENCE data as also DO_NOT_USE then this data is in the combined image. This region should be in the combined image. The longer term fix is to get the MIRI team to set the NON_SCIENCE pixels in the flat field reference file to DO_NOT_USE + NON_SCIENCE. We can then take out this code.
________________________________
From: James Davies <[email protected]>
Sent: Wednesday, January 13, 2021 12:19 PM
To: spacetelescope/jwst <[email protected]>
Cc: Jane Morrison <[email protected]>; Author <[email protected]>
Subject: [EXT]Re: [spacetelescope/jwst] JP-1694: fix outlier detection for MIRI imaging (#5601)
External Email
External Email
@jdavies-st commented on this pull request.
Thanks for the excellent debug here Jane! Just some questions and comments below.
________________________________
In jwst/outlier_detection/outlier_detection.py<#5601 (comment)>:
@@ -421,7 +421,8 @@ def flag_cr(sci_image, blot_image, **pars):
#
# Model the noise and create a CR mask
diff_noise = np.abs(sci_data - blot_data)
- ta = np.sqrt(np.abs(blot_data + subtracted_background) + err_data ** 2)
+ #ta = np.sqrt(np.abs(blot_data + subtracted_background) + err_data ** 2)
Please delete the commented-out line.
________________________________
In jwst/outlier_detection/outlier_detection.py<#5601 (comment)>:
@@ -406,7 +406,7 @@ def flag_cr(sci_image, blot_image, **pars):
exptime = sci_image.meta.exposure.exposure_time
sci_data = sci_image.data * exptime
- blot_data = blot_image.data * exptime
+ blot_data = (blot_image.data +subtracted_background) * exptime
Get a space after that + operator.
________________________________
In jwst/flatfield/flat_field.py<#5601 (comment)>:
+ # Find all pixels in the flat that have a DQ value of NON_SCIENCE
+ # add the DO_NOT_USE flag to these pixels. We don't want to use these pixels
+ # in futher steps
+ flag_nonsci = np.bitwise_and(science.dq, dqflags.pixel['NON_SCIENCE']).astype(np.bool)
+ science.dq[flag_nonsci] = np.bitwise_or(science.dq[flag_nonsci], dqflags.pixel['DO_NOT_USE'])
Adding NON_SCIENCE doesn't seem to have any effect on the issue described in the ticket. I.e. in the dataset from the ticket, running calwebb_image3 starting with the _cal.fits files as they are resolves the issue with just the changes in outlier_detection, without having to rerun flat_field on the input data.
So what is the purpose of this section of code? Currently the NON_SCIENCE area in MIRI data gets set to zero in the resampled _i2d.fits output. I.e., I don't see a problem with what is happening currently.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<#5601 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AENCOWIVWXUAW3PGCQG5NSDSZXW5JANCNFSM4V3EOVYQ>.
|
|
The preferred method is for outlier detection to have good_bits = DO_NOT_USE only. If we do not go back to the flat field step and flag NON_SCIENCE data as also DO_NOT_USE then this data is in the combined image. This region should NOT be in the combined image. The longer term fix is to get the MIRI team to set the NON_SCIENCE pixels in the flat field reference file to DO_NOT_USE + NON_SCIENCE. We can then take out this code. |
|
That makes sense Jane. Thanks. I think what I'm seeing is that jwst/jwst/resample/resample_step.py Line 19 in 2bbb677 |
|
@jdavies-st |
|
@jdavies-st Can we ignore the Code style check - it seems to be related to a long line in a file that is not even part of the PR. |
I fixed the long line issue in a PR I recently merged. So it'll be gone once this gets merged with master. |
There was a problem hiding this comment.
Is the comment by @jdavies-st accurate, in that flagging the NON_SCIENCE pixels with DO_NOT_USE makes no difference to the final product? Does it make any difference in outlier_detection?
Ah, never mind. Just caught up on the rest of the comment thread here, which verifies that we do in fact want the NON_SCIENCE to have DO_NOT_USE added to them. So that means the only thing that needs changing yet is the change log.
| @@ -1,6 +1,14 @@ | |||
| 0.18.2 (unreleased) | |||
| =================== | |||
|
|
|||
| flat_field | |||
There was a problem hiding this comment.
Minor nit, but we like to keep the change entires in alphabetical order of step/pipeline name, so these should go below cube_build in the list.
There was a problem hiding this comment.
Ok I think I finally have the change log correct. I just edited it on GitHub
There was a problem hiding this comment.
Yep, looks good to me now.
There was a problem hiding this comment.
I am giving this PR my highest approval. 🥇
🌟 🌟 🌟 🌟
This not only fixes the reported issue for MIRI, but it also makes outlier_detection actually work for all the other instruments.
For instance, here's NIRCam imaging with 3 NRCB5 detectors at different roll angles.
On the left is the i2d file for jwst 0.18.1. On the right is this branch. Outliers are actually detected and removed.
Rockon @jemorrison 🎉
jdavies-st
changed the title
| @@ -1,6 +1,14 @@ | |||
| 0.18.2 (unreleased) | |||
| =================== | |||
|
|
|||
| flat_field | |||
There was a problem hiding this comment.
Yep, looks good to me now.
|
As soon as CI tests complete successfully I'll merge (hopefully in time for the nightly regtest run). |
This PR goes a long way to fixing combined data for MIRI.
the parameter good_bits has to be set to (~DO_NOT_USE+NON_SCIENCE) to ignore both NON_SCIENCE data and BAD data set with DO_NOT_USE.
In addition the noise image used to flag outliers was updated to account for the background subtracted region
Resolves #5332 / JP-1694
Also resolves #5528 / JP-1806