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

Use aperture flux for photometric calibration

    XMLWordPrintable

Details

    • Story
    • Status: Done
    • Resolution: Done
    • None
    • pipe_tasks
    • None

    Description

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

      Attachments

        Issue Links

          Activity

            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?

            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?
            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).

            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).

            Thanks nlust. 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.

            swinbank John Swinbank added a comment - Thanks nlust . 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.
            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)

            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)

            Thanks nlust. Merged to master.

            swinbank John Swinbank added a comment - Thanks nlust . Merged to master.

            People

              swinbank John Swinbank
              swinbank John Swinbank
              Nate Lust
              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:

                Jenkins

                  No builds found.