Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-12968

Include INTERP+CR pixels in coadds

    Details

    • Story Points:
      1
    • Team:
      External

      Description

      I think we agreed at the 2018-12-11 Monday Meeting that it's better to included interpolated CR pixels in the coadd (since the interpolation is pretty good) rather than leave them out and get lots more pixels masked with INEXACT_PSF.

      Paul Price, if you're in a position to do this quickly before the next HSC release, please steal it. Otherwise I'll try to get to it later.

        Attachments

          Issue Links

            Activity

            Hide
            rhl Robert Lupton added a comment -

            CR for sure as the interpolation is decent (it should be better!). I'm less convinced that we should include INTERP

            Show
            rhl Robert Lupton added a comment - CR for sure as the interpolation is decent (it should be better!). I'm less convinced that we should include INTERP
            Hide
            jbosch Jim Bosch added a comment - - edited

            The current configuration actually has:

            badMaskPlanes = ['BAD', 'EDGE', 'SAT', 'INTERP', 'NO_DATA']
            

            so the reason we're not propagating the interpolated CR pixels is actually because they're also masked with INTERP.

            I don't think our low-level code can currently handle an expression like "include pixels with INTERP only if they also have CR", so I think the appropriate thing to do here is to remove INTERP from the list and rely on the (existing) rejection of BAD, SAT, and NO_DATA to remove pixels for which we don't expect interpolation to be good.

            Show
            jbosch Jim Bosch added a comment - - edited The current configuration actually has: badMaskPlanes = ['BAD', 'EDGE', 'SAT', 'INTERP', 'NO_DATA'] so the reason we're not propagating the interpolated CR pixels is actually because they're also masked with INTERP. I don't think our low-level code can currently handle an expression like "include pixels with INTERP only if they also have CR", so I think the appropriate thing to do here is to remove INTERP from the list and rely on the (existing) rejection of BAD, SAT, and NO_DATA to remove pixels for which we don't expect interpolation to be good.
            Hide
            rhl Robert Lupton added a comment -

            That sounds reasonable, but we do need to revisit that code (was that part of Pim's work on Statistics?) at some point.

            Show
            rhl Robert Lupton added a comment - That sounds reasonable, but we do need to revisit that code (was that part of Pim's work on Statistics ?) at some point.
            Hide
            jbosch Jim Bosch added a comment -

            Here's a shot of coadds before (left) and after (right) removing INTRP from the coaddition badMaskPlanes configuration. Two things jump out:

            • CRs have essentially the same pixel values, but are masked differently as expected (now INTERP+CR; was INEXACT_PSF).
            • Saturated/bleed pixels are being handled differently as well. I'm not sure what's going on here, but I suspect it's an interaction with the known no-good-pixels bug in WarpCompare.

            Given the latter, I think I should hold merging and do another test once the fix for that is merged (though I'd be interested to hear Yusra AlSayyad's thoughts on that).

            Show
            jbosch Jim Bosch added a comment - Here's a shot of coadds before (left) and after (right) removing INTRP from the coaddition badMaskPlanes configuration. Two things jump out: CRs have essentially the same pixel values, but are masked differently as expected (now INTERP+CR; was INEXACT_PSF). Saturated/bleed pixels are being handled differently as well. I'm not sure what's going on here, but I suspect it's an interaction with the known no-good-pixels bug in WarpCompare. Given the latter, I think I should hold merging and do another test once the fix for that is merged (though I'd be interested to hear Yusra AlSayyad 's thoughts on that).
            Hide
            yusra Yusra AlSayyad added a comment - - edited

            The no-good-pixels bug fix went in yesterday: see DM-12697.

            WarpCompare does not run by default; these are SafeClip coadds. To use warpCompare you need to specify that in a config file. Instructions here: https://paper.dropbox.com/doc/CompareWarp-Evaluation-OCKPjQtffrEP6fkJPAt9A

            Show
            yusra Yusra AlSayyad added a comment - - edited The no-good-pixels bug fix went in yesterday: see DM-12697 . WarpCompare does not run by default; these are SafeClip coadds. To use warpCompare you need to specify that in a config file. Instructions here: https://paper.dropbox.com/doc/CompareWarp-Evaluation-OCKPjQtffrEP6fkJPAt9A
            Hide
            yusra Yusra AlSayyad added a comment -

            Note that if you are making changes to obs_subaru/config. You now need to do that in 3 places instead of 2:

            1. config/assembleCoadd.py
            2. config/safeClipAssembleCoadd.py
            3. config/compareWarpAssembleCoadd.py

            Show
            yusra Yusra AlSayyad added a comment - Note that if you are making changes to obs_subaru/config. You now need to do that in 3 places instead of 2: 1. config/assembleCoadd.py 2. config/safeClipAssembleCoadd.py 3. config/compareWarpAssembleCoadd.py
            Hide
            jbosch Jim Bosch added a comment -

            Thanks for the corrections and the tip on config/compareWarpAssembleCoadd.py - I had missed that one. I think I've found how this affects SafeClip's logic, and I think the change is harmless; it's just affecting which epochs contribute to areas that are are masked SAT on every epoch, and the resulting area on the coadd is always masked SAT.

            I'll run a pair of coadds with and without these configs with warpCompare to make sure the same is true there.

            Show
            jbosch Jim Bosch added a comment - Thanks for the corrections and the tip on config/compareWarpAssembleCoadd.py - I had missed that one. I think I've found how this affects SafeClip's logic, and I think the change is harmless; it's just affecting which epochs contribute to areas that are are masked SAT on every epoch, and the resulting area on the coadd is always masked SAT. I'll run a pair of coadds with and without these configs with warpCompare to make sure the same is true there.
            Hide
            yusra Yusra AlSayyad added a comment -

            When testing WarpCompare, use master pipe_tasks/obs_subaru if possible

            Show
            yusra Yusra AlSayyad added a comment - When testing WarpCompare, use master pipe_tasks/obs_subaru if possible
            Hide
            jbosch Jim Bosch added a comment -

            The WarpCompare results look quite similar to the SafeClip ones, and that made me think I had misdiagnosed the behavior of the new configs around bright stars.

            I had - the problem is that we have some pixels masked with INTRP in the single-epoch images with no additional bit set to indicate why they were interpolated. I think I'd call that a bug, and I've opened DM-12985 to keep it separate from the coadd configuration.

            Because the config change proposed by this ticket does make the bright stars on the coadd look a bit worse (because of those INTRP-only pixels), I'm inclined to block this merge on DM-12985. If that takes too long to sort out, however, we may want to press forward with this one on the principle that avoiding overzealous INEXACT_PSF masking outweighs any damage done to saturated stars.

            Show
            jbosch Jim Bosch added a comment - The WarpCompare results look quite similar to the SafeClip ones, and that made me think I had misdiagnosed the behavior of the new configs around bright stars. I had - the problem is that we have some pixels masked with INTRP in the single-epoch images with no additional bit set to indicate why they were interpolated. I think I'd call that a bug, and I've opened DM-12985 to keep it separate from the coadd configuration. Because the config change proposed by this ticket does make the bright stars on the coadd look a bit worse (because of those INTRP-only pixels), I'm inclined to block this merge on DM-12985 . If that takes too long to sort out, however, we may want to press forward with this one on the principle that avoiding overzealous INEXACT_PSF masking outweighs any damage done to saturated stars.
            Hide
            jbosch Jim Bosch added a comment - - edited

            Yusra AlSayyad, since you've already looked at most of this config-only change, could you do the formal review?

            I think the most important question is whether there is anywhere else I need to modify these configuration options.

            Note that final merge of this is blocked on DM-12985, but that shouldn't actually require any changes to this ticket.

            Show
            jbosch Jim Bosch added a comment - - edited Yusra AlSayyad , since you've already looked at most of this config-only change, could you do the formal review? I think the most important question is whether there is anywhere else I need to modify these configuration options. Note that final merge of this is blocked on DM-12985 , but that shouldn't actually require any changes to this ticket.
            Hide
            yusra Yusra AlSayyad added a comment -

            Looks good.

            1) No other obs_packages have overrides, but double check that the default badMaskPlanes for all cameras is desirable:
            self.badMaskPlanes = ["NO_DATA", "BAD", "CR", ]
            https://github.com/lsst/pipe_tasks/blob/master/python/lsst/pipe/tasks/assembleCoadd.py#L139

            2) A search for INTRP in github won't turn up your commit message spelled INTERP. The commit message is clear regardless.

            Show
            yusra Yusra AlSayyad added a comment - Looks good. 1) No other obs_packages have overrides, but double check that the default badMaskPlanes for all cameras is desirable: self.badMaskPlanes = ["NO_DATA", "BAD", "CR", ] https://github.com/lsst/pipe_tasks/blob/master/python/lsst/pipe/tasks/assembleCoadd.py#L139 2) A search for INTRP in github won't turn up your commit message spelled INTERP. The commit message is clear regardless.
            Hide
            yusra Yusra AlSayyad added a comment -

            Here's an N_Image (visits contributing to each pixel for a few configs left to right:

            1) PRE DM-12968 WarpCompare HSC /datasets/hsc/repo/rerun/private/yusra/psfMatching/DM-12697/deepCoadd/HSC-G/9813/5,8_nImage.fits
            2) POST DM-12968 WarpCompare HSC /datasets/hsc/repo/rerun/private/yusra/psfMatching/w_2017_48/warpCompare_DM-12968/deepCoadd/HSC-G/9813/5,8_nImage.fits
            3) POST DM-12968 SafeClip HSC /datasets/hsc/repo/rerun/private/yusra/psfMatching/w_2017_48/safeClipOverride/deepCoadd/HSC-G/9813/5,8_nImage.fits
            4) POST DM-12968 SafeClip ALL OTHER CAMERAS (see my comment #1 above): /datasets/hsc/repo/rerun/private/yusra/psfMatching/w_2017_48/safeClip/deepCoadd/HSC-G/9813/5,8_nImage.fits

            Show
            yusra Yusra AlSayyad added a comment - Here's an N_Image (visits contributing to each pixel for a few configs left to right: 1) PRE DM-12968 WarpCompare HSC /datasets/hsc/repo/rerun/private/yusra/psfMatching/ DM-12697 /deepCoadd/HSC-G/9813/5,8_nImage.fits 2) POST DM-12968 WarpCompare HSC /datasets/hsc/repo/rerun/private/yusra/psfMatching/w_2017_48/warpCompare_ DM-12968 /deepCoadd/HSC-G/9813/5,8_nImage.fits 3) POST DM-12968 SafeClip HSC /datasets/hsc/repo/rerun/private/yusra/psfMatching/w_2017_48/safeClipOverride/deepCoadd/HSC-G/9813/5,8_nImage.fits 4) POST DM-12968 SafeClip ALL OTHER CAMERAS (see my comment #1 above): /datasets/hsc/repo/rerun/private/yusra/psfMatching/w_2017_48/safeClip/deepCoadd/HSC-G/9813/5,8_nImage.fits
            Hide
            jbosch Jim Bosch added a comment -

            I hadn't realized the defaults for other cameras were in such bad shape - thanks for bringing that to my attention.

            In response, I've added some commits to this ticket that moves the (new) HSC defaults into AssembleCoaddTask and removes the overrides in pipe_drivers and obs_subaru. I also moved the badMaskPlanes config option itself from CoaddBaseTask to AssembleCoaddTask, since we weren't actually using it in MakeCoaddTempExpTask.

            Yusra AlSayyad, do you mind taking another look, since the changes themselves are now fairly different from what they were before?

            Show
            jbosch Jim Bosch added a comment - I hadn't realized the defaults for other cameras were in such bad shape - thanks for bringing that to my attention. In response, I've added some commits to this ticket that moves the (new) HSC defaults into AssembleCoaddTask and removes the overrides in pipe_drivers and obs_subaru. I also moved the badMaskPlanes config option itself from CoaddBaseTask to AssembleCoaddTask , since we weren't actually using it in MakeCoaddTempExpTask . Yusra AlSayyad , do you mind taking another look, since the changes themselves are now fairly different from what they were before?
            Hide
            yusra Yusra AlSayyad added a comment -

            OK to merge Jim Bosch

            Show
            yusra Yusra AlSayyad added a comment - OK to merge Jim Bosch

              People

              • Assignee:
                jbosch Jim Bosch
                Reporter:
                jbosch Jim Bosch
                Reviewers:
                Yusra AlSayyad
                Watchers:
                Jim Bosch, Paul Price, Robert Lupton, Yusra AlSayyad
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel