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

Enable aperture correction on coadd processing

    Details

    • Type: Bug
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: meas_base, pipe_tasks
    • Labels:
      None

      Description

      Aperture corrections are now coadded, so we can enable aperture corrections in measurements done on coadds.

        Attachments

          Issue Links

            Activity

            Hide
            jbosch Jim Bosch added a comment -

            A pair of tiny changes to enable aperture correction in meas_base and pipe_tasks; it looks like aperture correction was previously disabled here before because corrections weren't available on coadds when the most recent edition of aperture correction apply code was ported from HSC.

            commit 56bcf1f4da4c84f35cc17f69ce7c1941c50f3094
            Author: Jim Bosch <jbosch@astro.princeton.edu>
            Date:   Fri Feb 5 18:25:16 2016 -0500
             
                Apply aperture corrections in forced photometry.
             
            diff --git a/python/lsst/meas/base/forcedPhotImage.py b/python/lsst/meas/base/forcedPhotImage.py
            index 3d98f94..a5f88c4 100644
            --- a/python/lsst/meas/base/forcedPhotImage.py
            +++ b/python/lsst/meas/base/forcedPhotImage.py
            @@ -57,9 +57,7 @@ class ProcessImageForcedConfig(lsst.pex.config.Config):
                     )
             
                 def setDefaults(self):
            -        # coadds do not yet include include aperture correction data,
            -        # but that is planned, so might as well warn when it occurs
            -        self.measurement.doApplyApCorr = "noButWarn"
            +        self.measurement.doApplyApCorr = "yes"
             
             ## @addtogroup LSST_task_documentation
             ## @{
            

            commit f8e30ae15f3dde315246a7fc0696f2d66e75fc81
            Author: Jim Bosch <jbosch@astro.princeton.edu>
            Date:   Fri Feb 5 18:17:58 2016 -0500
             
                Enable aperture correction in multiband coadd processing.
             
            diff --git a/python/lsst/pipe/tasks/multiBand.py b/python/lsst/pipe/tasks/multiBand.py
            index 9879481..ad4d803 100644
            --- a/python/lsst/pipe/tasks/multiBand.py
            +++ b/python/lsst/pipe/tasks/multiBand.py
            @@ -471,6 +471,7 @@ class MeasureMergedCoaddSourcesConfig(Config):
                     # The following line must be set if clipped pixel flags are to be added to the output table
                     # The clipped mask plane is added by running SafeClipAssembleCoaddTask
                     self.measurement.plugins['base_PixelFlags'].masksFpAnywhere = ['CLIPPED']
            +        self.measurement.doApplyApCorr = "yes"
             
             class MeasureMergedCoaddSourcesTask(CmdLineTask):
                 """Measure sources using the merged catalog of detections
            

            Show
            jbosch Jim Bosch added a comment - A pair of tiny changes to enable aperture correction in meas_base and pipe_tasks; it looks like aperture correction was previously disabled here before because corrections weren't available on coadds when the most recent edition of aperture correction apply code was ported from HSC. commit 56bcf1f4da4c84f35cc17f69ce7c1941c50f3094 Author: Jim Bosch <jbosch@astro.princeton.edu> Date: Fri Feb 5 18:25:16 2016 -0500   Apply aperture corrections in forced photometry.   diff --git a/python/lsst/meas/base/forcedPhotImage.py b/python/lsst/meas/base/forcedPhotImage.py index 3d98f94..a5f88c4 100644 --- a/python/lsst/meas/base/forcedPhotImage.py +++ b/python/lsst/meas/base/forcedPhotImage.py @@ -57,9 +57,7 @@ class ProcessImageForcedConfig(lsst.pex.config.Config): ) def setDefaults(self): - # coadds do not yet include include aperture correction data, - # but that is planned, so might as well warn when it occurs - self.measurement.doApplyApCorr = "noButWarn" + self.measurement.doApplyApCorr = "yes" ## @addtogroup LSST_task_documentation ## @{ commit f8e30ae15f3dde315246a7fc0696f2d66e75fc81 Author: Jim Bosch <jbosch@astro.princeton.edu> Date: Fri Feb 5 18:17:58 2016 -0500   Enable aperture correction in multiband coadd processing.   diff --git a/python/lsst/pipe/tasks/multiBand.py b/python/lsst/pipe/tasks/multiBand.py index 9879481..ad4d803 100644 --- a/python/lsst/pipe/tasks/multiBand.py +++ b/python/lsst/pipe/tasks/multiBand.py @@ -471,6 +471,7 @@ class MeasureMergedCoaddSourcesConfig(Config): # The following line must be set if clipped pixel flags are to be added to the output table # The clipped mask plane is added by running SafeClipAssembleCoaddTask self.measurement.plugins['base_PixelFlags'].masksFpAnywhere = ['CLIPPED'] + self.measurement.doApplyApCorr = "yes" class MeasureMergedCoaddSourcesTask(CmdLineTask): """Measure sources using the merged catalog of detections
            Hide
            swinbank John Swinbank added a comment -

            It would be helpful to be a bit more explicit about the trigger for this happening now. Was this blocked on DM-833?

            ProcessImageForcedConfig is used not just by ForcedPhotCoaddTask but also by ForcedPhotCcdTask. Is the intention to change behaviour there as well?

            It's unfortunate that we don't have any tests which demonstrate that setting these options is actually doing anything. I don't think it's necessary to expand the scope of this ticket to cover writing more extensive test cases, but if you could see a way to drop in some quick check that we've actually changed some numbers here that would be good. Even simply hard-coding a test that the configuration is set in the relevant tasks would be better than nothing – at the very least, it forces us to stop and think and update the tests when changing defaults in future.

            Show
            swinbank John Swinbank added a comment - It would be helpful to be a bit more explicit about the trigger for this happening now. Was this blocked on DM-833 ? ProcessImageForcedConfig is used not just by ForcedPhotCoaddTask but also by ForcedPhotCcdTask . Is the intention to change behaviour there as well? It's unfortunate that we don't have any tests which demonstrate that setting these options is actually doing anything. I don't think it's necessary to expand the scope of this ticket to cover writing more extensive test cases, but if you could see a way to drop in some quick check that we've actually changed some numbers here that would be good. Even simply hard-coding a test that the configuration is set in the relevant tasks would be better than nothing – at the very least, it forces us to stop and think and update the tests when changing defaults in future.
            Hide
            jbosch Jim Bosch added a comment -

            It would be helpful to be a bit more explicit about the trigger for this happening now. Was this blocked on DM-833?

            That's my interpretation of the comment I found in meas_base; I'm pretty sure DM-436 was completed before DM-833, and the latter should have enabled aperture correction on coadds but did not.

            ProcessImageForcedConfig is used not just by ForcedPhotCoaddTask but also by ForcedPhotCcdTask. Is the intention to change behaviour there as well?

            Yes. I don't think the state of ForcedPhotCcdTask vis-a-vis aperture corrections was really intentional before - single-visit forced photometry is often overlooked since the lack of deblending there prevents it from being useful right right now. But enabling aperture corrections there makes sense as well, since we should always have corrections we can apply at that stage in the processing.

            It's unfortunate that we don't have any tests which demonstrate that setting these options is actually doing anything. I don't think it's necessary to expand the scope of this ticket to cover writing more extensive test cases, but if you could see a way to drop in some quick check that we've actually changed some numbers here that would be good. Even simply hard-coding a test that the configuration is set in the relevant tasks would be better than nothing – at the very least, it forces us to stop and think and update the tests when changing defaults in future.

            I'm thinking of adding some "unit test" scripts to ci_hsc that would verify that that the aperture correction columns have been added to the appropriate output catalog datasets.

            Show
            jbosch Jim Bosch added a comment - It would be helpful to be a bit more explicit about the trigger for this happening now. Was this blocked on DM-833 ? That's my interpretation of the comment I found in meas_base; I'm pretty sure DM-436 was completed before DM-833 , and the latter should have enabled aperture correction on coadds but did not. ProcessImageForcedConfig is used not just by ForcedPhotCoaddTask but also by ForcedPhotCcdTask. Is the intention to change behaviour there as well? Yes. I don't think the state of ForcedPhotCcdTask vis-a-vis aperture corrections was really intentional before - single-visit forced photometry is often overlooked since the lack of deblending there prevents it from being useful right right now. But enabling aperture corrections there makes sense as well, since we should always have corrections we can apply at that stage in the processing. It's unfortunate that we don't have any tests which demonstrate that setting these options is actually doing anything. I don't think it's necessary to expand the scope of this ticket to cover writing more extensive test cases, but if you could see a way to drop in some quick check that we've actually changed some numbers here that would be good. Even simply hard-coding a test that the configuration is set in the relevant tasks would be better than nothing – at the very least, it forces us to stop and think and update the tests when changing defaults in future. I'm thinking of adding some "unit test" scripts to ci_hsc that would verify that that the aperture correction columns have been added to the appropriate output catalog datasets.
            Hide
            swinbank John Swinbank added a comment - - edited

            I'm all in favour of adding more tests to ci_hsc, but we should be aware that it's not currently deployed as part of CI and likely to be extensively rewritten before it gets that far. It's a handy tool for us to use locally, but I don't think it's a substitute for other testing.

            On reflection, I think even a noddy test (does that make sense in American? "Simplistic", I guess) which literally checks that the aperture corrections are enabled in the task configs would be worthwhile. That would, for example, have answered my second point above (if the unit tests were asserting that it should be enabled for ForcedPhotCcd, it would unambiguously be intentional), and it would cover us against trivial mistakes like DM-3159.

            That said, I think you can go ahead and merge, whether or not you decide to add such a test.

            Show
            swinbank John Swinbank added a comment - - edited I'm all in favour of adding more tests to ci_hsc , but we should be aware that it's not currently deployed as part of CI and likely to be extensively rewritten before it gets that far. It's a handy tool for us to use locally, but I don't think it's a substitute for other testing. On reflection, I think even a noddy test (does that make sense in American? "Simplistic", I guess) which literally checks that the aperture corrections are enabled in the task configs would be worthwhile. That would, for example, have answered my second point above (if the unit tests were asserting that it should be enabled for ForcedPhotCcd , it would unambiguously be intentional), and it would cover us against trivial mistakes like DM-3159 . That said, I think you can go ahead and merge, whether or not you decide to add such a test.
            Hide
            jbosch Jim Bosch added a comment -

            I don't actually understand where you're proposing I add the test; if we add it to the actual Tasks, won't that prevent someone from changing the config when they actually have a good reason to? (I can't think of one right now, but that's true for 95% of our configuration parameters).

            I do think we need to start adding tests to ci_hsc, or if not that, one of the validate_something packages I'm starting to see for other tests. We have a lot of things that can only be tested at that stage, and those are precisely the things that we've had the most trouble getting right in HSC data releases. I'm content with it being ci_hsc for now, even if it's not being run automatically, because it's sufficiently easy to run manually that we do run it a lot in practice. But if you think there's a better short-term option I'm happy to go another direction. Note that I'd also like to write some similar, full-pipeline-level checks for DM-5084.

            Show
            jbosch Jim Bosch added a comment - I don't actually understand where you're proposing I add the test; if we add it to the actual Tasks, won't that prevent someone from changing the config when they actually have a good reason to? (I can't think of one right now, but that's true for 95% of our configuration parameters). I do think we need to start adding tests to ci_hsc, or if not that, one of the validate_something packages I'm starting to see for other tests. We have a lot of things that can only be tested at that stage, and those are precisely the things that we've had the most trouble getting right in HSC data releases. I'm content with it being ci_hsc for now, even if it's not being run automatically, because it's sufficiently easy to run manually that we do run it a lot in practice. But if you think there's a better short-term option I'm happy to go another direction. Note that I'd also like to write some similar, full-pipeline-level checks for DM-5084 .
            Hide
            jbosch Jim Bosch added a comment -

            Merged to master. I've added some checks to verify that aperture corrections are being applied to ci_hsc. It turns out the code is running but it's failing on every source, but I'll investigate that and put in further checks on DM-5109.

            Show
            jbosch Jim Bosch added a comment - Merged to master. I've added some checks to verify that aperture corrections are being applied to ci_hsc. It turns out the code is running but it's failing on every source, but I'll investigate that and put in further checks on DM-5109 .
            Hide
            swinbank John Swinbank added a comment -

            Sorry: I just found this lurking at the bottom of my inbox.

            My suggestion was simply that you could demonstrate that the configuration really was being applied properly – and that it's application to ForcedPhotCcd is intentional – by doing something like:

            assertEqual(lsst.meas.base.forcedPhotCcd.ForcedPhotCcdConfig().measurement.doApplyApCorr, "yes")
            

            in the meas_base tests. I don't think this is of any great importance, though.

            Show
            swinbank John Swinbank added a comment - Sorry: I just found this lurking at the bottom of my inbox. My suggestion was simply that you could demonstrate that the configuration really was being applied properly – and that it's application to ForcedPhotCcd is intentional – by doing something like: assertEqual(lsst.meas.base.forcedPhotCcd.ForcedPhotCcdConfig().measurement.doApplyApCorr, "yes") in the meas_base tests. I don't think this is of any great importance, though.

              People

              • Assignee:
                jbosch Jim Bosch
                Reporter:
                jbosch Jim Bosch
                Reviewers:
                John Swinbank
                Watchers:
                Jim Bosch, John Swinbank
              • Votes:
                0 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: