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

Add slot for calibration flux

    Details

      Description

      This is a port of HSC-1005.

        Attachments

          Issue Links

            Activity

            Hide
            swinbank John Swinbank added a comment - - edited

            Hi Nate – would you mind performing a quick review of this?

            This is a port of HSC-1005. Specifically, we are adding a new slot, CalibFlux, which we use to record the flux measurement to be used for calibration. There are changes in six repositories:

            Note that HSC-1005 includes a few other tweaks to obs_subaru related to using an aperture flux for photometric calibration. We're not currently doing that on LSST, so they don't apply here. We'll consider how to bring them over as part of DM-3108.

            Show
            swinbank John Swinbank added a comment - - edited Hi Nate – would you mind performing a quick review of this? This is a port of HSC-1005. Specifically, we are adding a new slot, CalibFlux , which we use to record the flux measurement to be used for calibration. There are changes in six repositories: On afw 's tickets/DM-3106 we define the new slot and add some tests to ensure that it (and other flux slots) successfully provide basic functionality; On meas_base 's tickets/DM-3106 we arrange for measurement tasks to store an appropriate value in the new slot; On tickets/ DM-3106 in ip_diffim , meas_algorithms and meas_modelfit we simply tweak some task configurations to avoid recording a CalibFlux when it's not necessary; On pipe_tasks ` tickets/DM-3106 we set the photoCal task to use the CalibFlux slot rather than the PSF flux measurement. Note that HSC-1005 includes a few other tweaks to obs_subaru related to using an aperture flux for photometric calibration. We're not currently doing that on LSST, so they don't apply here. We'll consider how to bring them over as part of DM-3108 .
            Hide
            nlust Nate Lust added a comment -

            This all looks good to me, with two minor comments that I suggest but don't require both of which are in afw tests/testSourceTable.py:

            • rename baseName to base, to be more consistent with how slots and such a documented in other places
            • consider using python .format() for your string formatting. It will help future proof things, and be a bit easier to read.
              Like I said these are just suggestions, with the first being stronger than the second. Other than that I would say good to go
            Show
            nlust Nate Lust added a comment - This all looks good to me, with two minor comments that I suggest but don't require both of which are in afw tests/testSourceTable.py: rename baseName to base, to be more consistent with how slots and such a documented in other places consider using python .format() for your string formatting. It will help future proof things, and be a bit easier to read. Like I said these are just suggestions, with the first being stronger than the second. Other than that I would say good to go
            Hide
            swinbank John Swinbank added a comment -

            Nate – thanks for the review! A brief response:

            rename baseName to base, to be more consistent with how slots and such a documented in other places

            On reflection, I agree that using "baseName" is a poor choice. However, I also don't like "base" – it's normally used to indicate the output of a measurement plugin defined in meas_base (cf the prefix "ext" for meas_extensions_*). Instead, I will change it to use "afw_Test", since this is a test in the afw package.

            consider using python .format() for your string formatting. It will help future proof things, and be a bit easier to read.

            You may consider it considered!

            For what it's worth, I find it hard to get really excited one way or the other. When this was discussed on HipChat a few months ago (in the Tavern, so carrying no formal weight), there was no real consensus about which was the better approach; while a couple of folks are strongly in favour of one approach or the other (no prizes for guessing names), I think the general attitude was that we expect both to be supported in Python for the long term and that which is more readable depends chiefly on context.

            Here, % formatting is used extensively in the other afw tests, while I don't think any of them call str.format(); for consistency, I think we should stick with the former.

            Show
            swinbank John Swinbank added a comment - Nate – thanks for the review! A brief response: rename baseName to base, to be more consistent with how slots and such a documented in other places On reflection, I agree that using "baseName" is a poor choice. However, I also don't like "base" – it's normally used to indicate the output of a measurement plugin defined in meas_base (cf the prefix "ext" for meas_extensions_* ). Instead, I will change it to use "afw_Test" , since this is a test in the afw package. consider using python .format() for your string formatting. It will help future proof things, and be a bit easier to read. You may consider it considered! For what it's worth, I find it hard to get really excited one way or the other. When this was discussed on HipChat a few months ago (in the Tavern, so carrying no formal weight), there was no real consensus about which was the better approach; while a couple of folks are strongly in favour of one approach or the other (no prizes for guessing names), I think the general attitude was that we expect both to be supported in Python for the long term and that which is more readable depends chiefly on context. Here, % formatting is used extensively in the other afw tests, while I don't think any of them call str.format() ; for consistency, I think we should stick with the former.

              People

              • Assignee:
                swinbank John Swinbank
                Reporter:
                swinbank John Swinbank
                Reviewers:
                Nate Lust
                Watchers:
                John Swinbank, Nate Lust
              • Votes:
                0 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel