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

Refactor ForcedPhotImageTask (and children) per RFC-352

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: meas_base
    • Labels:
      None

      Description

      Implementing RFC-352  prepping for the supertask conversion in two stages. DM-2639 changed the TaskRunner to call a Task's runDataRef method.  During code review Russell Owen points out that ForcedPhotImageTask (and its children ForcedPhotCcdTask and ForcedPhotCoaddTask  would be simple to refactor.

        Attachments

          Issue Links

            Activity

            Hide
            czw Christopher Waters added a comment -

            Hopefully a quick review.

            Small refactor of ForcedPhotImageTask to add a run method. ForcedPhotCcd and ForcedPhotCoadd will both inherit this method.

            Show
            czw Christopher Waters added a comment - Hopefully a quick review. Small refactor of ForcedPhotImageTask to add a run method. ForcedPhotCcd and ForcedPhotCoadd will both inherit this method.
            Hide
            rowen Russell Owen added a comment -

            Thank for doing this. I had a few small requested changes on github.

            Show
            rowen Russell Owen added a comment - Thank for doing this. I had a few small requested changes on github.
            Hide
            yusra Yusra AlSayyad added a comment -

            Only because I'm a watcher on this ticket: This implementation looks incomplete because you're just passing the DataRef to run.

            You can call the IdFactory in runDataRef and pass it to run. Do any of the children's attachFootprints()  actually need the DataRef? You might be able to change that API.

            Show
            yusra Yusra AlSayyad added a comment - Only because I'm a watcher on this ticket: This implementation looks incomplete because you're just passing the DataRef to run. You can call the IdFactory in runDataRef and pass it to run. Do any of the children's attachFootprints()  actually need the DataRef? You might be able to change that API.
            Hide
            yusra Yusra AlSayyad added a comment -

            OK, I see the answer to my question is yes a child needs to fetch the heavy footprints from disk. So maybe everything down and including the call to `attachFootprints()` should be part of `runDataRef` and run just does

                self.measurement.run(measCat, exposure, refCat, refWcs, 
                if self.config.doApCorr:
                        self.applyApCorr.run(
                            catalog=measCat,
                            apCorrMap=exposure.getInfo().getApCorrMap()
                        )
                self.catalogCalculation.run(measCat)
            

            Show
            yusra Yusra AlSayyad added a comment - OK, I see the answer to my question is yes a child needs to fetch the heavy footprints from disk. So maybe everything down and including the call to `attachFootprints()` should be part of `runDataRef` and run just does self.measurement.run(measCat, exposure, refCat, refWcs, if self.config.doApCorr: self.applyApCorr.run( catalog=measCat, apCorrMap=exposure.getInfo().getApCorrMap() ) self.catalogCalculation.run(measCat)
            Hide
            rowen Russell Owen added a comment -

            Good catch. Sorry I missed that and I agree with Yusra AlSayyad. I'll update my comments on github.

            Show
            rowen Russell Owen added a comment - Good catch. Sorry I missed that and I agree with Yusra AlSayyad . I'll update my comments on github.
            Hide
            czw Christopher Waters added a comment - - edited

            That makes sense to isolate the DataRef from run().

            I'm running a new version through Jenkins to make sure I have everything correct in the new code, and will update the branch once that passes.

            Show
            czw Christopher Waters added a comment - - edited That makes sense to isolate the DataRef from run(). I'm running a new version through Jenkins to make sure I have everything correct in the new code, and will update the branch once that passes.

              People

              Assignee:
              czw Christopher Waters
              Reporter:
              yusra Yusra AlSayyad
              Reviewers:
              Russell Owen
              Watchers:
              Christopher Waters, Russell Owen, Yusra AlSayyad
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  CI Builds

                  No builds found.