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

Use aperture flux for photometric calibration

    Details

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

      Description

      This is a port of work performed on HSC but without a ticket. Relevant commits are:

        Attachments

          Issue Links

            Activity

            Hide
            swinbank John Swinbank added a comment -

            Nate, this is a minor extension to DM-3106, which you're already looking at, and is based on a design which you help specify. Do you mind taking a quick look?

            Show
            swinbank John Swinbank added a comment - Nate, this is a minor extension to DM-3106 , which you're already looking at, and is based on a design which you help specify. Do you mind taking a quick look?
            Hide
            nlust Nate Lust added a comment -

            This looks good, with one exception. In the python and c++ code you use .1f as a string repeatedly within lines of code. It would be better to avoid these magic numbers and define it in a header file or config file somewhere. This will allow us to change the value in only one location (possibly two if you need to change in python and c++ separately) if we start caring about higher levels of precision (i.e. both 3.2 and 3.28).

            Show
            nlust Nate Lust added a comment - This looks good, with one exception. In the python and c++ code you use .1f as a string repeatedly within lines of code. It would be better to avoid these magic numbers and define it in a header file or config file somewhere. This will allow us to change the value in only one location (possibly two if you need to change in python and c++ separately) if we start caring about higher levels of precision (i.e. both 3.2 and 3.28).
            Hide
            swinbank John Swinbank added a comment -

            Thanks Nate Lust. I see your point. I've attempted to address it by adding a static member function to ApertureFluxAlgorithm which generates field names and is exposed to Python. This means that formatting is defined in exactly one place. I'm not sure that I'd want to go further in terms of exposing this to configuration etc, as it seems like that would be adding a lot of complexity to what should be a simple operation – I'd rather cross that bridge when we come to it.

            I'm only semi-happy with the approach adopted, as I don't really like burdening the ApertureFluxAlgorithm interface in this way. Would you mind taking a look to see what you think and if you have any better ideas? Commit is here; I'll squash it into the rest of the PR if & when we decide if this is the approach we actually want to take.

            Show
            swinbank John Swinbank added a comment - Thanks Nate Lust . I see your point. I've attempted to address it by adding a static member function to ApertureFluxAlgorithm which generates field names and is exposed to Python. This means that formatting is defined in exactly one place. I'm not sure that I'd want to go further in terms of exposing this to configuration etc, as it seems like that would be adding a lot of complexity to what should be a simple operation – I'd rather cross that bridge when we come to it. I'm only semi-happy with the approach adopted, as I don't really like burdening the ApertureFluxAlgorithm interface in this way. Would you mind taking a look to see what you think and if you have any better ideas? Commit is here ; I'll squash it into the rest of the PR if & when we decide if this is the approach we actually want to take.
            Hide
            nlust Nate Lust added a comment -

            I can't really think of a better place for that function. It seems like the right idea to have that functionality defined in on place. Leaving it where it is seems good, as it can always be moved later if we create a better place for it to live. This seems good to merge provided it passes tests after rebase (sorry the review took so long)

            Show
            nlust Nate Lust added a comment - I can't really think of a better place for that function. It seems like the right idea to have that functionality defined in on place. Leaving it where it is seems good, as it can always be moved later if we create a better place for it to live. This seems good to merge provided it passes tests after rebase (sorry the review took so long)
            Hide
            swinbank John Swinbank added a comment -

            Thanks Nate Lust. Merged to master.

            Show
            swinbank John Swinbank added a comment - Thanks Nate Lust . Merged to master.

              People

              • Assignee:
                swinbank John Swinbank
                Reporter:
                swinbank John Swinbank
                Reviewers:
                Nate Lust
                Watchers:
                Jim Bosch, John Swinbank, Nate Lust, Paul Price, Robert Lupton
              • Votes:
                0 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel