Uploaded image for project: 'Request For Comments'
  1. Request For Comments
  2. RFC-510

Rename table getters and setters for fluxes as per RFC-322

    Details

    • Type: RFC
    • Status: Implemented
    • Resolution: Done
    • Component/s: DM
    • Labels:
      None
    • Location:
      here

      Description

      RFC-322 has us renaming flux fields in tables, e.g. *Flux_flux to *Flux_instFlux for fluxes in counts, and then adding *Flux_flux to mean physical flux (e.g. in Jy).

      However, source tables also have getters and setters for instrument flux, such as getPsfFlux. At present (before implementing RFC-322) there is an obvious mapping between field names in source tables and the accessor names, e.g. getPsfFlux returns field psfFlux_flux. If we implement RFC-322 without also renaming the getters and setters then that linkage is broken. For example getPsfFlux will return data from psfFlux_instFlux instead of psfFlux_flux. I fear users will find that confusing and upsetting.

      Thus I propose that as we implement RFC-322 we also rename the getters and setters: get/set*Flux becomes get/set*InstFlux, for example getPsfFlux becomes. getPsfInstFlux.

      I further propose that we do not add getters and setters for physical flux. By not defining them we reduce the danger that old code will read the wrong thing from source tables: getPsfFlux will simply not exist, instead of returning the wrong thing. (We may decide to add them later, if we really miss them, but that decision is out of scope for this RFC.)

      I have taken the liberty of adding as watchers those who weighed in on RFC-322

        Attachments

          Issue Links

            Activity

            Hide
            swinbank John Swinbank added a comment -

            The above got confusing (at least to me), but I don't think it need be. Let me summarise:

            • Per RFC-322, all measurement algorithms will be updated so that they write (instrumental) fluxes to fields named algorithmName_instFlux, where algorithmName is e.g. base_PsfFlux, base_GaussianFlux, etc.
            • Per this RFC, the convenience getters/setters provided by flux slots will be renamed so that they become SourceTable.getSlotnameInstFlux, where SlotName is e.g. Psf, Ap, etc.
            • No additional fields (e.g. to represent physical fluxes) will be added to tables, and it follows that no new getters or setters will be added to access those fields which don't exist.
            • No changes will be made to algorithm names.

            In future, we might want to add physical fluxes into source tables directly, and perhaps they'll be accompanied by getters & setters, or perhaps we'll provide some convenience function that calculates them on the sky given the instrumental flux and an appropriate calib. However, no requirements or coherent design for this has been advanced yet. Brainstorming how it might be done is out of scope for this RFC, and should not block the work described above.

            In future, we might also want to redesign the way that slots are handled from a technical point of view. That redesign will likely play into considerations around adding physical fluxes to tables, and what it means for getters, setters, etc. Again, the potential of a redesign should not block the work described above.

            Show
            swinbank John Swinbank added a comment - The above got confusing (at least to me), but I don't think it need be. Let me summarise: Per RFC-322 , all measurement algorithms will be updated so that they write (instrumental) fluxes to fields named algorithmName_instFlux , where algorithmName is e.g. base_PsfFlux , base_GaussianFlux , etc. Per this RFC, the convenience getters/setters provided by flux slots will be renamed so that they become SourceTable.getSlotnameInstFlux , where SlotName is e.g. Psf , Ap , etc. No additional fields (e.g. to represent physical fluxes) will be added to tables, and it follows that no new getters or setters will be added to access those fields which don't exist. No changes will be made to algorithm names. In future, we might want to add physical fluxes into source tables directly, and perhaps they'll be accompanied by getters & setters, or perhaps we'll provide some convenience function that calculates them on the sky given the instrumental flux and an appropriate calib. However, no requirements or coherent design for this has been advanced yet. Brainstorming how it might be done is out of scope for this RFC, and should not block the work described above. In future, we might also want to redesign the way that slots are handled from a technical point of view. That redesign will likely play into considerations around adding physical fluxes to tables, and what it means for getters, setters, etc. Again, the potential of a redesign should not block the work described above.
            Hide
            Parejkoj John Parejko added a comment -

            Thanks for further clarifying my declarifications, John Swinbank. I agree with all of your above.

            Russell Owen: do you think the RFC needs to be modified to make John's points clear? You don't explicitly mention the slots themselves in the RFC text.

            Show
            Parejkoj John Parejko added a comment - Thanks for further clarifying my declarifications, John Swinbank . I agree with all of your above. Russell Owen : do you think the RFC needs to be modified to make John's points clear? You don't explicitly mention the slots themselves in the RFC text.
            Hide
            rowen Russell Owen added a comment -

            Thank you for the clarifications. It sounds like we should proceed with this change.

            Show
            rowen Russell Owen added a comment - Thank you for the clarifications. It sounds like we should proceed with this change.
            Hide
            rowen Russell Owen added a comment -

            John Parejko I see no reason to mention slots as this RFC is only about renaming the getters and setters. Am I missing something?

            Also, if you have no objections, I would like to reassign this RFC to you, since you are doing the work.

            Show
            rowen Russell Owen added a comment - John Parejko I see no reason to mention slots as this RFC is only about renaming the getters and setters. Am I missing something? Also, if you have no objections, I would like to reassign this RFC to you, since you are doing the work.
            Hide
            Parejkoj John Parejko added a comment - - edited

            After thoroughly confusing myself, here's the list of things in afw Source.h (generated from Source.h.m4) that are changing:

            getThingFluxKey()->getThingInstFluxKey()
            getThingFluxErrKey()->getThingInstFluxErrKey()
            getThingFlux()->getThingInstFlux()
            getThingFluxErr()->getThingInstFluxErr()
            

            and that are not changing:

            getThingFluxSlot()
            defineThingFlux()
            getThingFluxDefinition()
            hasThingFluxSlot()
            getThingFluxFlagKey()
            FluxSlotDefinition (class used to implement the above)
            

            Note that most of the above "unchanged" have been deprecated since 2014, and so are ripe for deletion.

            Show
            Parejkoj John Parejko added a comment - - edited After thoroughly confusing myself, here's the list of things in afw Source.h (generated from Source.h.m4) that are changing: getThingFluxKey()->getThingInstFluxKey() getThingFluxErrKey()->getThingInstFluxErrKey() getThingFlux()->getThingInstFlux() getThingFluxErr()->getThingInstFluxErr() and that are not changing: getThingFluxSlot() defineThingFlux() getThingFluxDefinition() hasThingFluxSlot() getThingFluxFlagKey() FluxSlotDefinition (class used to implement the above) Note that most of the above "unchanged" have been deprecated since 2014, and so are ripe for deletion.

              People

              • Assignee:
                Parejkoj John Parejko
                Reporter:
                rowen Russell Owen
                Watchers:
                Colin Slater, Jim Bosch, John Parejko, John Swinbank, Kian-Tat Lim, Michael Wood-Vasey, Paul Price, Russell Owen, Simon Krughoff
              • Votes:
                0 Vote for this issue
                Watchers:
                9 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Planned End:

                  Summary Panel