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

make forced and SFM interfaces more consistent

    XMLWordPrintable

    Details

    • Story Points:
      1
    • Sprint:
      Science Pipelines DM-S15-6
    • Team:
      Data Release Production

      Description

      From Russell Owen:

      SimpleMeasurementTask.run and ForcedMeasurementTask.run now both take a source catalog, but the two use the opposite order for the first two arguments (one has the catalog first, the other has the exposure first)

        Attachments

          Activity

          No builds found.
          jbosch Jim Bosch created issue -
          Hide
          jbosch Jim Bosch added a comment -

          Russell Owen, I think this addresses all of the forced photometry interfaces. It also includes a fix that gets Kron working (I think we can just skip the branch for DM-1954), and a mostly-unrelated minor change in afw to fix an error message I encountered while working on the issue.

          Changes in afw, meas_base, meas_modelfit, and meas_extensions_photometryKron, all on tickets/DM-3459 (as linked from Jira).

          Show
          jbosch Jim Bosch added a comment - Russell Owen , I think this addresses all of the forced photometry interfaces. It also includes a fix that gets Kron working (I think we can just skip the branch for DM-1954 ), and a mostly-unrelated minor change in afw to fix an error message I encountered while working on the issue. Changes in afw, meas_base, meas_modelfit, and meas_extensions_photometryKron, all on tickets/ DM-3459 (as linked from Jira).
          jbosch Jim Bosch made changes -
          Field Original Value New Value
          Reviewers Russell Owen [ rowen ]
          Status To Do [ 10001 ] In Review [ 10004 ]
          rowen Russell Owen made changes -
          Component/s afw [ 10714 ]
          Component/s meas_extensions_photometryKron [ 12318 ]
          Component/s meas_modelfit [ 11411 ]
          Hide
          rowen Russell Owen added a comment -

          Thank you very much for making these changes, and so quickly. Everything looks great (nice fix in afw), though I did want to make one suggestion about a pre-existing issue in meas_base.

          From the documentation of ForcedMeasurementTask it sounds as if most users that call generateSources will then immediately have to call attachTransformedFootprints (which will be overridden if needed). If so, this sounds a bit clumsy to me. I suggest you have generateSources peform that call, with a boolean flag to disable if necessary. Or if the decision to call attachTransformedFootprints is intrinsic to the task, then make subtasks override generateSources. If you do make this change then is it also reasonable to make attachTransformedFootprints private (by prefixing an underscore)?

          Thank you very much for the extra note on documentation for the measCat argument of ForcedMeasurementTask.run about using generateSources. I was hoping you add that.

          Show
          rowen Russell Owen added a comment - Thank you very much for making these changes, and so quickly. Everything looks great (nice fix in afw), though I did want to make one suggestion about a pre-existing issue in meas_base. From the documentation of ForcedMeasurementTask it sounds as if most users that call generateSources will then immediately have to call attachTransformedFootprints (which will be overridden if needed). If so, this sounds a bit clumsy to me. I suggest you have generateSources peform that call, with a boolean flag to disable if necessary. Or if the decision to call attachTransformedFootprints is intrinsic to the task, then make subtasks override generateSources. If you do make this change then is it also reasonable to make attachTransformedFootprints private (by prefixing an underscore)? Thank you very much for the extra note on documentation for the measCat argument of ForcedMeasurementTask.run about using generateSources. I was hoping you add that.
          rowen Russell Owen made changes -
          Status In Review [ 10004 ] Reviewed [ 10101 ]
          Hide
          jbosch Jim Bosch added a comment -

          The key point here is that many (perhaps most) real users of ForcedMeasurementTask will in fact want to provide their own custom attachFootprints implementation, because not doing so leads to poor results (because the forced measurements are not deblended). In fact, it's in the CmdLineTask drivers - ForcedPhotImageTask, ForcedPhotCcdTask and ForcedPhotCoaddTask - that this method (actually named attachFootprints there) should be overridden; the goal is for those CmdLineTasks to be subclassed, with all subclasses using ForcedMeasurementTask as-is without subclassing it, but providing their own implementation of attachFootprints. We consider it the job of those classes to generate the Footprints because in general they have information that allow them to create better Footprints that is never passed down to ForocedMeasurementTask.

          In fact, we're reetained attachTransformedFootprints method on ForcedMeasurementTask mostly just for use in unit tests - it's also used by ForcedPhotCcdTask at present, but that's an algorithmic problem that will ultimately need to be addressed.

          Show
          jbosch Jim Bosch added a comment - The key point here is that many (perhaps most) real users of ForcedMeasurementTask will in fact want to provide their own custom attachFootprints implementation, because not doing so leads to poor results (because the forced measurements are not deblended). In fact, it's in the CmdLineTask drivers - ForcedPhotImageTask , ForcedPhotCcdTask and ForcedPhotCoaddTask - that this method (actually named attachFootprints there) should be overridden; the goal is for those CmdLineTasks to be subclassed, with all subclasses using ForcedMeasurementTask as-is without subclassing it, but providing their own implementation of attachFootprints . We consider it the job of those classes to generate the Footprints because in general they have information that allow them to create better Footprints that is never passed down to ForocedMeasurementTask . In fact, we're reetained attachTransformedFootprints method on ForcedMeasurementTask mostly just for use in unit tests - it's also used by ForcedPhotCcdTask at present, but that's an algorithmic problem that will ultimately need to be addressed.
          Hide
          rowen Russell Owen added a comment - - edited

          OK. I wonder if it makes sense to try to document how footprints are usually attached in generateSources or somewhere similar? It sounds complicated.

          One other minor point: generateSources would be clearer as generateMeasCat. We can live with it, but I'm happy to make that change if it's acceptable and you don't want to bother.

          Show
          rowen Russell Owen added a comment - - edited OK. I wonder if it makes sense to try to document how footprints are usually attached in generateSources or somewhere similar? It sounds complicated. One other minor point: generateSources would be clearer as generateMeasCat. We can live with it, but I'm happy to make that change if it's acceptable and you don't want to bother.
          Hide
          jbosch Jim Bosch added a comment -

          I was wondering about that rename myself. If you're in favor, I'll do it.

          Show
          jbosch Jim Bosch added a comment - I was wondering about that rename myself. If you're in favor, I'll do it.
          Hide
          jbosch Jim Bosch added a comment -

          I've renamed generateSources to generateMeasCat, added some documentation, run CI, and merged to master.

          Show
          jbosch Jim Bosch added a comment - I've renamed generateSources to generateMeasCat , added some documentation, run CI, and merged to master.
          jbosch Jim Bosch made changes -
          Resolution Done [ 10000 ]
          Status Reviewed [ 10101 ] Done [ 10002 ]

            People

            Assignee:
            jbosch Jim Bosch
            Reporter:
            jbosch Jim Bosch
            Reviewers:
            Russell Owen
            Watchers:
            Jim Bosch, Russell Owen
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:

                Jenkins

                No builds found.