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

Some flux fields are not getting their units set

    XMLWordPrintable

    Details

    • Story Points:
      1
    • Sprint:
      DRP F18-5
    • Team:
      Data Release Production

      Description

      Since the flux -> instFlux name change of DM-10302, some backwards compatibility issues with older catalogs were revealed and thought to be fixed on DM-15857. The idea was to set aliases for all flux fields, identifying them by their units: any field with unit "count", "dn", or "adu" gets an alias set. This relies on the proper units having been set for all flux fields in the catalog. I have just found several cases (two very important ones!) where units are not being set:

      (Field['D'](name="modelfit_CModel_flux", doc="flux from the final cmodel fit"), Key<D>(offset=984, nElements=1))
      (Field['D'](name="modelfit_CModel_fluxErr", doc="flux uncertainty from the final cmodel fit"), Key<D>(offset=992, nElements=1))
      (Field['D'](name="modelfit_CModel_initial_flux_inner", doc="flux within the fit region, with no extrapolation"), Key<D>(offset=928, nElements=1))
      (Field['D'](name="modelfit_CModel_dev_flux_inner", doc="flux within the fit region, with no extrapolation"), Key<D>(offset=976, nElements=1))
      (Field['D'](name="modelfit_CModel_exp_flux_inner", doc="flux within the fit region, with no extrapolation"), Key<D>(offset=1376, nElements=1))
      (Field['D'](name="modelfit_CModel_flux_inner", doc="flux within the fit region, with no extrapolation"), Key<D>(offset=1000, nElements=1))
      

      As a result, we currently (with w_2018_40) can't even read in an older coadd catalog as a butler.get throws with:

      Field or subfield with name 'modelfit_CModel_instFlux' not found with type 'D'. {0}
      lsst::pex::exceptions::NotFoundError: 'Field or subfield with name 'modelfit_CModel_instFlux' not found with type 'D'.'
      

      This needs to be fixed for future processing, but still needs a workaround for old catalogs that do not have the units set properly.

        Attachments

          Issue Links

            Activity

            Hide
            jbosch Jim Bosch added a comment -

            This seems like it should be a pretty high priority; I can probably take a look today or tomorrow, but it'd be very nice to start with a failing unit test.  Lauren MacArthur, could you provide one?

            Show
            jbosch Jim Bosch added a comment - This seems like it should be a pretty high priority; I can probably take a look today or tomorrow, but it'd be very nice to start with a failing unit test.  Lauren MacArthur , could you provide one?
            Hide
            lauren Lauren MacArthur added a comment -

            Sure, I can give that a go...

            Show
            lauren Lauren MacArthur added a comment - Sure, I can give that a go...
            Hide
            lauren Lauren MacArthur added a comment - - edited

            Just found one non-CModel case:

            (Field['D'](name="deblend_psfFlux", doc="If deblended-as-psf, the PSF flux"), Key<D>(offset=64, nElements=1))
            

            Also, should this really be deblend_psfFlux_flux? If so, and it's not controversial, I could add it to the list of to-do's on DM-16070 (implementation ticket of RFC-534).

            Show
            lauren Lauren MacArthur added a comment - - edited Just found one non-CModel case: (Field[ 'D' ](name= "deblend_psfFlux" , doc= "If deblended-as-psf, the PSF flux" ), Key<D>(offset= 64 , nElements= 1 )) Also, should this really be deblend_psfFlux_flux ? If so, and it's not controversial, I could add it to the list of to-do's on DM-16070 (implementation ticket of RFC-534 ).
            Hide
            jbosch Jim Bosch added a comment -

            Also, should this really be deblend_psfFlux_flux? If so, and it's not controversial, I could add it to the list of to-do's on DM-16070

            Yes, please do (though now it should probably be something like deblend_psf_instFlux).  I'm going to ignore that field on this ticket because I'm quite confident nothing is using it, and the usual alias won't even work for it because of its current non-standard structure.

            I'll both make sure we add aliases to the CModel fields and start writing them with the correct units on this ticket.

            Show
            jbosch Jim Bosch added a comment - Also, should this really be deblend_psfFlux_flux ? If so, and it's not controversial, I could add it to the list of to-do's on DM-16070 Yes, please do (though now it should probably be something like deblend_psf_instFlux ).  I'm going to ignore that field on this ticket because I'm quite confident nothing is using it, and the usual alias won't even work for it because of its current non-standard structure. I'll both make sure we add aliases to the CModel fields and start writing them with the correct units on this ticket.
            Hide
            lauren Lauren MacArthur added a comment -

            Will do.

            I'm going to ignore that field on this ticket because I'm quite confident nothing is using it, and the usual alias won't even work for it because of its current non-standard structure.

            That's why I didn't include that field in the unittest

            Show
            lauren Lauren MacArthur added a comment - Will do. I'm going to ignore that field on this ticket because I'm quite confident nothing is using it, and the usual alias won't even work for it because of its current non-standard structure. That's why I didn't include that field in the unittest
            Hide
            jbosch Jim Bosch added a comment -

            Jenkins is off and running, but afw tests pass (with no changes!) so I think this is done:

            https://github.com/lsst/afw/pull/406

            https://github.com/lsst/meas_modelfit/pull/70/files

             

            Show
            jbosch Jim Bosch added a comment - Jenkins is off and running, but afw tests pass (with no changes!) so I think this is done: https://github.com/lsst/afw/pull/406 https://github.com/lsst/meas_modelfit/pull/70/files  
            Hide
            lauren Lauren MacArthur added a comment -

            Looks good, thanks for getting to this so quickly.

            Normally, I would test this by putting it through the ringer of my compareCoaddAnalysis.py scripts and making it handle a v2 and v3 catalog comparison...but I don't have an easy/expeditious way to get an updated stack at the moment (due to DM-16129 & I'm getting permission errors on Eli Rykoff's w_2018_40 stack, not sure what changed there...)  Anyway, I am confident enough in the tests – and this is a serious enough issue – that I'm happy for you to merge with a successful Jenkins build.

            Show
            lauren Lauren MacArthur added a comment - Looks good, thanks for getting to this so quickly. Normally, I would test this by putting it through the ringer of my compareCoaddAnalysis.py scripts and making it handle a v2 and v3 catalog comparison...but I don't have an easy/expeditious way to get an updated stack at the moment (due to DM-16129 & I'm getting permission errors on Eli Rykoff 's w_2018_40 stack, not sure what changed there...)  Anyway, I am confident enough in the tests – and this is a serious enough issue – that I'm happy for you to merge with a successful Jenkins build.

              People

              Assignee:
              jbosch Jim Bosch
              Reporter:
              lauren Lauren MacArthur
              Reviewers:
              Lauren MacArthur
              Watchers:
              Eli Rykoff, Jim Bosch, John Parejko, Lauren MacArthur, Yusra AlSayyad
              Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved: