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

            yusra Yusra AlSayyad created issue -
            yusra Yusra AlSayyad made changes -
            Field Original Value New Value
            Epic Link DM-14448 [ 80390 ]
            yusra Yusra AlSayyad made changes -
            Link This issue is child task of DM-11098 [ DM-11098 ]
            yusra Yusra AlSayyad made changes -
            Risk Score 0
            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.
            czw Christopher Waters made changes -
            Reviewers Russell Owen [ rowen ]
            Status To Do [ 10001 ] In Review [ 10004 ]
            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.
            rowen Russell Owen made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            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.
            czw Christopher Waters made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]
            czw Christopher Waters made changes -
            Story Points 1 2

              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: