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

All NaNs in coord_ra and coord_dec columns in deepCoadd forced src tables

    XMLWordPrintable

    Details

      Description

      In recent runs of the stack through multiBandDriver.py, the persisted forced src tables for the coadds are not getting ra and dec set properly (all entries for the coord_ra and coord_dec columns are NaN). Looking back at a run in mid-Nov, 2016, these numbers were indeed set properly in the forced tables. Assuming this was not intentional, track down the cause and fix it such that these values get set properly for the persisted forced src tables.

        Attachments

          Issue Links

            Activity

            Hide
            swinbank John Swinbank added a comment - - edited

            I'm guessing this originates in DM-8210, where, on 448121a4, the copyColumns config which included coord_ra and coord_dec was removed as a duplicate. We do still have a copyColumns in ForcedMeasurementConfig, but it doesn't mention these fields.

            Reading DM-8210, I'm confused about what's actually supposed to be going on here. Jim Bosch, maybe you could clarify the intention?

            Show
            swinbank John Swinbank added a comment - - edited I'm guessing this originates in DM-8210 , where, on 448121a4 , the copyColumns config which included coord_ra and coord_dec was removed as a duplicate. We do still have a copyColumns in ForcedMeasurementConfig , but it doesn't mention these fields. Reading DM-8210 , I'm confused about what's actually supposed to be going on here. Jim Bosch , maybe you could clarify the intention?
            Hide
            jbosch Jim Bosch added a comment -

            I think I was assuming that the forced photometry tasks would explicitly set the coord fields from the reference catalog, but it appears that's not the case - reference catalog centroids are now added to the forced catalogs via the base_TransformedCentroid plugins in meas_base, which basically act like a centroiding algorithm that just looks up the coord fields from the reference catalog and copies it into the centroid fields in the forced catalog. It appears that in the past we were actually relying on copyColumns to do that.

            So, one way to fix this would be to just add these to copyColumns in the ForcedMeasurementTask config defaults. Another would be to add a forced-measurement version of the base_SkyCoord algorithm in meas_base, which is what we use in single-frame measurement to fill the coord fields. Or it might be best to have base_TransformedCentroid do that itself; I can't think of a reason why it shouldn't. I think any of these is fine - they're all a bit ugly, IMO, and maybe copyColumns is worst just because it can lead to the kinds of problems we saw in DM-8210 if we use it to overwrite existing columns - but the ultimate reason this is difficutl is because I think it's intrinsically tricky (but of course incredibly useful) to have the coord columns at all in what is otherwise a table containing only raw, uncalibrated quantities.

            Show
            jbosch Jim Bosch added a comment - I think I was assuming that the forced photometry tasks would explicitly set the coord fields from the reference catalog, but it appears that's not the case - reference catalog centroids are now added to the forced catalogs via the base_TransformedCentroid plugins in meas_base, which basically act like a centroiding algorithm that just looks up the coord fields from the reference catalog and copies it into the centroid fields in the forced catalog. It appears that in the past we were actually relying on copyColumns to do that. So, one way to fix this would be to just add these to copyColumns in the ForcedMeasurementTask config defaults. Another would be to add a forced-measurement version of the base_SkyCoord algorithm in meas_base , which is what we use in single-frame measurement to fill the coord fields. Or it might be best to have base_TransformedCentroid do that itself; I can't think of a reason why it shouldn't. I think any of these is fine - they're all a bit ugly, IMO, and maybe copyColumns is worst just because it can lead to the kinds of problems we saw in DM-8210 if we use it to overwrite existing columns - but the ultimate reason this is difficutl is because I think it's intrinsically tricky (but of course incredibly useful) to have the coord columns at all in what is otherwise a table containing only raw, uncalibrated quantities.
            Hide
            swinbank John Swinbank added a comment -

            Thanks Jim!

            Do we care about deblend_nchild? It looks to have been dropped in the same commit as the RA/dec.

            Show
            swinbank John Swinbank added a comment - Thanks Jim! Do we care about deblend_nchild ? It looks to have been dropped in the same commit as the RA/dec.
            Hide
            jbosch Jim Bosch added a comment - - edited

            I think that was correctly dropped - there is no deblending on CCD-level forced photometry right now, so I think that field would be more misleading than helpful.

            Show
            jbosch Jim Bosch added a comment - - edited I think that was correctly dropped - there is no deblending on CCD-level forced photometry right now, so I think that field would be more misleading than helpful.
            Hide
            lauren Lauren MacArthur added a comment -

            Ummm...I care! I've been reading it in from the meas catalogs. They also have a few other fields I've been using that don't get persisted/copied to the forced cats, so I may not be able to avoid reading in both for my current use case, but it would be nice to have (assuming there's no fundamental reason to leave it out that I'm missing).

            Show
            lauren Lauren MacArthur added a comment - Ummm...I care! I've been reading it in from the meas catalogs. They also have a few other fields I've been using that don't get persisted/copied to the forced cats, so I may not be able to avoid reading in both for my current use case, but it would be nice to have (assuming there's no fundamental reason to leave it out that I'm missing).
            Hide
            price Paul Price added a comment -

            price@pap-laptop:~/LSST/meas_base (tickets/DM-9556=) $ git sub-patch
            commit 8b143a2191cdbe980bfb725296cc743cb969ea2a
            Author: Paul Price <price@astro.princeton.edu>
            Date:   Fri Mar 31 11:48:21 2017 -0400
             
                forcedMeasurement: copy coordinates
                
                Forced measurement catalogs currently have no RA,Dec. These
                used to be copied from the reference catalog, but commit
                448121a4 removed this behaviour.  Restoring it.
                
                Deliberately not restoring the 'deblend_nchild' since there is
                no deblending in forced measurement so that copying that field
                would be more misleading than helpful.
             
            diff --git a/python/lsst/meas/base/forcedMeasurement.py b/python/lsst/meas/base/forcedMeasurement.py
            index 52dcbb3..18879f1 100644
            --- a/python/lsst/meas/base/forcedMeasurement.py
            +++ b/python/lsst/meas/base/forcedMeasurement.py
            @@ -163,7 +163,8 @@ class ForcedMeasurementConfig(BaseMeasurementConfig):
             
                 copyColumns = lsst.pex.config.DictField(
                     keytype=str, itemtype=str, doc="Mapping of reference columns to source columns",
            -        default={"id": "objectId", "parent": "parentObjectId"}
            +        default={"id": "objectId", "parent": "parentObjectId",
            +                 "coord_ra": "coord_ra", "coord_dec": "coord_dec"}
                 )
             
                 checkUnitsParseStrict = lsst.pex.config.Field(
            

            Show
            price Paul Price added a comment - price@pap-laptop:~/LSST/meas_base (tickets/DM-9556=) $ git sub-patch commit 8b143a2191cdbe980bfb725296cc743cb969ea2a Author: Paul Price <price@astro.princeton.edu> Date: Fri Mar 31 11:48:21 2017 -0400   forcedMeasurement: copy coordinates Forced measurement catalogs currently have no RA,Dec. These used to be copied from the reference catalog, but commit 448121a4 removed this behaviour. Restoring it. Deliberately not restoring the 'deblend_nchild' since there is no deblending in forced measurement so that copying that field would be more misleading than helpful.   diff --git a/python/lsst/meas/base/forcedMeasurement.py b/python/lsst/meas/base/forcedMeasurement.py index 52dcbb3..18879f1 100644 --- a/python/lsst/meas/base/forcedMeasurement.py +++ b/python/lsst/meas/base/forcedMeasurement.py @@ -163,7 +163,8 @@ class ForcedMeasurementConfig(BaseMeasurementConfig): copyColumns = lsst.pex.config.DictField( keytype=str, itemtype=str, doc="Mapping of reference columns to source columns", - default={"id": "objectId", "parent": "parentObjectId"} + default={"id": "objectId", "parent": "parentObjectId", + "coord_ra": "coord_ra", "coord_dec": "coord_dec"} ) checkUnitsParseStrict = lsst.pex.config.Field(
            Hide
            jbosch Jim Bosch added a comment -

            Looks good!

            Show
            jbosch Jim Bosch added a comment - Looks good!
            Hide
            swinbank John Swinbank added a comment -

            Please make sure that Lauren MacArthur agrees with the decision not to restore deblend_nchild before you merge.

            Show
            swinbank John Swinbank added a comment - Please make sure that Lauren MacArthur agrees with the decision not to restore deblend_nchild before you merge.
            Hide
            lauren Lauren MacArthur added a comment -

            I would really like it in the coadd forced catalogs (where deblending is done, correct?)

            Show
            lauren Lauren MacArthur added a comment - I would really like it in the coadd forced catalogs (where deblending is done, correct?)
            Hide
            price Paul Price added a comment -

            Revised:

            -price@pap-laptop:~/LSST/meas_base (tickets/DM-9556=) $ git sub-patch
            commit 94f876db450f3494ff1ef8d737c696a4586f7836
            Author: Paul Price <price@astro.princeton.edu>
            Date:   Fri Mar 31 11:48:21 2017 -0400
             
                forcedMeasurement: copy coordinates, deblend_nChild
                
                Forced measurement catalogs currently have no RA,Dec or
                deblend_nChild. These used to be copied from the reference
                catalog, but commit 448121a4 removed this behaviour.
                Restoring it.
             
            diff --git a/python/lsst/meas/base/forcedMeasurement.py b/python/lsst/meas/base/forcedMeasurement.py
            index 52dcbb3..1c97858 100644
            --- a/python/lsst/meas/base/forcedMeasurement.py
            +++ b/python/lsst/meas/base/forcedMeasurement.py
            @@ -163,7 +163,8 @@ class ForcedMeasurementConfig(BaseMeasurementConfig):
             
                 copyColumns = lsst.pex.config.DictField(
                     keytype=str, itemtype=str, doc="Mapping of reference columns to source columns",
            -        default={"id": "objectId", "parent": "parentObjectId"}
            +        default={"id": "objectId", "parent": "parentObjectId", "deblend_nChild": "deblend_nChild",
            +                 "coord_ra": "coord_ra", "coord_dec": "coord_dec"}
                 )
             
                 checkUnitsParseStrict = lsst.pex.config.Field(
            

            Show
            price Paul Price added a comment - Revised: -price@pap-laptop:~/LSST/meas_base (tickets/DM-9556=) $ git sub-patch commit 94f876db450f3494ff1ef8d737c696a4586f7836 Author: Paul Price <price@astro.princeton.edu> Date: Fri Mar 31 11:48:21 2017 -0400   forcedMeasurement: copy coordinates, deblend_nChild Forced measurement catalogs currently have no RA,Dec or deblend_nChild. These used to be copied from the reference catalog, but commit 448121a4 removed this behaviour. Restoring it.   diff --git a/python/lsst/meas/base/forcedMeasurement.py b/python/lsst/meas/base/forcedMeasurement.py index 52dcbb3..1c97858 100644 --- a/python/lsst/meas/base/forcedMeasurement.py +++ b/python/lsst/meas/base/forcedMeasurement.py @@ -163,7 +163,8 @@ class ForcedMeasurementConfig(BaseMeasurementConfig): copyColumns = lsst.pex.config.DictField( keytype=str, itemtype=str, doc="Mapping of reference columns to source columns", - default={"id": "objectId", "parent": "parentObjectId"} + default={"id": "objectId", "parent": "parentObjectId", "deblend_nChild": "deblend_nChild", + "coord_ra": "coord_ra", "coord_dec": "coord_dec"} ) checkUnitsParseStrict = lsst.pex.config.Field(
            Hide
            price Paul Price added a comment -

            Also need:

            price@pap-laptop:~/LSST/meas_modelfit (tickets/DM-9556=) $ git sub
            commit f2c37e1e4543ce5b5ffe670d2c149d8c426ca6b9
            Author: Paul Price <price@astro.princeton.edu>
            Date:   Fri Mar 31 13:35:24 2017 -0400
             
                tests: adapt to ForcedMeasurementConfig changes
                
                ForcedMeasurementConfig now by default copies fields that aren't
                present in our basic test catalogs, so set the list of columns
                to copy to what we do have (the same setting as was default previously).
             
             tests/testDoubleShapeletPsfApprox.py         | 1 +
             tests/testGeneralShapeletPsfApproxPlugins.py | 1 +
             2 files changed, 2 insertions(+)
            

            Plus similar changes in meas_extensions_photometryKron.

            Show
            price Paul Price added a comment - Also need: price@pap-laptop:~/LSST/meas_modelfit (tickets/DM-9556=) $ git sub commit f2c37e1e4543ce5b5ffe670d2c149d8c426ca6b9 Author: Paul Price <price@astro.princeton.edu> Date: Fri Mar 31 13:35:24 2017 -0400   tests: adapt to ForcedMeasurementConfig changes ForcedMeasurementConfig now by default copies fields that aren't present in our basic test catalogs, so set the list of columns to copy to what we do have (the same setting as was default previously).   tests/testDoubleShapeletPsfApprox.py | 1 + tests/testGeneralShapeletPsfApproxPlugins.py | 1 + 2 files changed, 2 insertions(+) Plus similar changes in meas_extensions_photometryKron.
            Hide
            price Paul Price added a comment -

            Jim Bosch, there are four new changes to review, required in tests of forced measurement because we changed the defaults to require a field which isn't present without some extra effort. Changes are in meas_modelfit, meas_extensions_photometryKron, meas_extensions_convolved and pipe_tasks. They're pretty simple, but please have a quick look at them before I merge.

            Show
            price Paul Price added a comment - Jim Bosch , there are four new changes to review, required in tests of forced measurement because we changed the defaults to require a field which isn't present without some extra effort. Changes are in meas_modelfit, meas_extensions_photometryKron, meas_extensions_convolved and pipe_tasks. They're pretty simple, but please have a quick look at them before I merge.
            Hide
            jbosch Jim Bosch added a comment -

            Looks fine. Too bad it's so fragile, but I don't see a simple way to fix that, and it's certainly out of scope here.

            Show
            jbosch Jim Bosch added a comment - Looks fine. Too bad it's so fragile, but I don't see a simple way to fix that, and it's certainly out of scope here.
            Hide
            price Paul Price added a comment -

            Thanks Jim.

            Merged to master.

            Show
            price Paul Price added a comment - Thanks Jim. Merged to master.

              People

              Assignee:
              price Paul Price
              Reporter:
              lauren Lauren MacArthur
              Reviewers:
              Jim Bosch
              Watchers:
              Jim Bosch, John Swinbank, Lauren MacArthur, Paul Price
              Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.