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

    XMLWordPrintable

Details

    • RFC
    • Status: Implemented
    • Resolution: Done
    • DM
    • None
    • 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

            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.

            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.
            Parejkoj John Parejko added a comment -

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

            rowen: 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.

            Parejkoj John Parejko added a comment - Thanks for further clarifying my declarifications, swinbank . I agree with all of your above. rowen : 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.
            rowen Russell Owen added a comment -

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

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

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

            rowen Russell Owen added a comment - Parejkoj 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.
            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.

            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

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

              Dates

                Created:
                Updated:
                Resolved:
                Planned End:

                Jenkins

                  No builds found.