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

Switch default measurement tasks to meas_base

    XMLWordPrintable

    Details

      Description

      We should set the default measurement task in ProcessImageTask to SingleFrameMeasurementTask, and note that SourceMeasurementTask and the old forced photometry drivers are deprecated.

        Attachments

          Issue Links

            Activity

            Hide
            jbosch Jim Bosch added a comment -

            A quibble: I'd like consistently just use lsst.meas.base.SingleFrameMeasurementTask rather than lsst.meas.base.sfm.SingleFrameMeasurementTask, since lsst/meas/base/__init__.py does from .sfm import * (and the same for all the other submodules of lsst.meas.base). While we don't have an official policy on this one way or the other, IMO including the submodule is unnecessarily verbose and slightly more fragile.

            If you're comfortable doing so, please consider using git rebase -i to squash the obs_sdss fixup commits into the commits that they fix.

            I think it would be fine to switch to version=1 in forcedPhot.py, but I'm not sure we need to. The larger problem here is that the ReferencesTask is not compatible with the new BaseReferencesTask API in meas_base, so at present it can only be used with the old forcedPhot.py task in pipe_tasks, which will eventually go away. I'm not convinced we need the special SDSS override for references - our CoaddSrcReferenceTask would work fine for SDSS, and I think it's probably better in that it doesn't rely on the database. We should capture that extra work on a new issue, and talk about the best way to resolve it with others.

            Show
            jbosch Jim Bosch added a comment - A quibble: I'd like consistently just use lsst.meas.base.SingleFrameMeasurementTask rather than lsst.meas.base.sfm.SingleFrameMeasurementTask , since lsst/meas/base/__init__.py does from .sfm import * (and the same for all the other submodules of lsst.meas.base ). While we don't have an official policy on this one way or the other, IMO including the submodule is unnecessarily verbose and slightly more fragile. If you're comfortable doing so, please consider using git rebase -i to squash the obs_sdss fixup commits into the commits that they fix. I think it would be fine to switch to version=1 in forcedPhot.py, but I'm not sure we need to. The larger problem here is that the ReferencesTask is not compatible with the new BaseReferencesTask API in meas_base, so at present it can only be used with the old forcedPhot.py task in pipe_tasks, which will eventually go away. I'm not convinced we need the special SDSS override for references - our CoaddSrcReferenceTask would work fine for SDSS, and I think it's probably better in that it doesn't rely on the database. We should capture that extra work on a new issue, and talk about the best way to resolve it with others.
            Hide
            pgee Perry Gee added a comment -

            Here is a summary of the differences in lsst_dm_stack_demo, which I have taken as my test of running the new meas_base framework on sdss data:

            These are caused by a change in the throw behavior:
            New nan measurement in GaussianFlux: 135
            New nan measurements in PsfFlux: 194

            These are caused by a change in the algorithm:
            Differences in GaussianFlux at the .1 level: 3875 The new measurement is about 15% lower
            Differences in GaussianFlux at the .15 level: 2493
            Differences in GaussianFlux at the .2 level: 77

            Differences in PsfFlux at the .005 level: 0

            Not sure what this is – probably a change in the reporting of xySigma
            New values which used to be nans in SdssShape_xySIgma: 4139

            537 which were extendedness 1.0 became 0.0 – this is probably caused by the change in Gaussian flux

            Do we call this the new baseline, or does it require additional investigation?

            Show
            pgee Perry Gee added a comment - Here is a summary of the differences in lsst_dm_stack_demo, which I have taken as my test of running the new meas_base framework on sdss data: These are caused by a change in the throw behavior: New nan measurement in GaussianFlux: 135 New nan measurements in PsfFlux: 194 These are caused by a change in the algorithm: Differences in GaussianFlux at the .1 level: 3875 The new measurement is about 15% lower Differences in GaussianFlux at the .15 level: 2493 Differences in GaussianFlux at the .2 level: 77 Differences in PsfFlux at the .005 level: 0 Not sure what this is – probably a change in the reporting of xySigma New values which used to be nans in SdssShape_xySIgma: 4139 537 which were extendedness 1.0 became 0.0 – this is probably caused by the change in Gaussian flux Do we call this the new baseline, or does it require additional investigation?
            Hide
            jbosch Jim Bosch added a comment -

            I asked about this (in general, not specifics) at the stand-up call, and was told that it's fine to just update the baseline, as long as some effort has gone into verifying that the new baseline is scientifically valid (which you've done), and interested parties have been warned (which I just did).

            I suspect the changes in the GaussianFlux are actually more due to not running aperture correction than anything else, unless you disabled CorrectFluxes in the old version before doing this comparison. In any case, I don't think it's something we need to worry about.

            Please note the desired delay in merging, however, as I mentioned in my recent message to you on HipChat (and in the lsst-data thread on the release).

            Show
            jbosch Jim Bosch added a comment - I asked about this (in general, not specifics) at the stand-up call, and was told that it's fine to just update the baseline, as long as some effort has gone into verifying that the new baseline is scientifically valid (which you've done), and interested parties have been warned (which I just did). I suspect the changes in the GaussianFlux are actually more due to not running aperture correction than anything else, unless you disabled CorrectFluxes in the old version before doing this comparison. In any case, I don't think it's something we need to worry about. Please note the desired delay in merging, however, as I mentioned in my recent message to you on HipChat (and in the lsst-data thread on the release).
            Hide
            jbosch Jim Bosch added a comment -

            Perry Gee We've been cleared to merge this.

            Show
            jbosch Jim Bosch added a comment - Perry Gee We've been cleared to merge this.
            Hide
            pgee Perry Gee added a comment -

            The difference in GaussianFlux is due to the change to use the "fixed" option instead of the the code in getGaussianFlux, which calculated the centroid and shape itself instead of using the slot values from previous stage in measurement.

            Note that there is also a new failure mode if the Shape slot is not set (note the change I made in testCoadds)

            Show
            pgee Perry Gee added a comment - The difference in GaussianFlux is due to the change to use the "fixed" option instead of the the code in getGaussianFlux, which calculated the centroid and shape itself instead of using the slot values from previous stage in measurement. Note that there is also a new failure mode if the Shape slot is not set (note the change I made in testCoadds)

              People

              Assignee:
              pgee Perry Gee
              Reporter:
              jbosch Jim Bosch
              Reviewers:
              Jim Bosch
              Watchers:
              Jim Bosch, Kian-Tat Lim, Perry Gee, Robyn Allsman [X] (Inactive)
              Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved: