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

            No builds found.
            rowen Russell Owen created issue -
            rowen Russell Owen made changes -
            Field Original Value New Value
            Link This issue is triggered by RFC-322 [ RFC-322 ]
            rowen Russell Owen made changes -
            Risk Score 0
            Parejkoj John Parejko added a comment -

            +1 As the one who is trying to implement this, and who started to implement it in the manner described here, I think this is the right approach.

            Parejkoj John Parejko added a comment - +1 As the one who is trying to implement this, and who started to implement it in the manner described here, I think this is the right approach.
            wmwood-vasey Michael Wood-Vasey added a comment - - edited

            Yes, this seems reasonable and necessary.

            May I request a rewording of the RFC to use the term "physical flux" or "flux in physical units" instead of "sky flux". The term "sky flux" to me, and I suspect to a significant portion of other astronomers, means the flux from the sky background.

            wmwood-vasey Michael Wood-Vasey added a comment - - edited Yes, this seems reasonable and necessary. May I request a rewording of the RFC to use the term "physical flux" or "flux in physical units" instead of "sky flux". The term "sky flux" to me, and I suspect to a significant portion of other astronomers, means the flux from the sky background.
            wmwood-vasey Michael Wood-Vasey made changes -
            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 sky 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 initially do *not* add getters and setters for sky flux. We can add them later if we really need them. By not defining them at first we reduce the danger that old code will suddenly start reading the wrong thing from source tables: {{getPsfFlux}} will simply not exist, instead of returning the wrong thing.

            I have taken the liberty of adding as watchers those who weighed in on DM-322
            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 sky 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 initially do *not* add getters and setters for sky flux. We can add them later if we really need them. By not defining them at first we reduce the danger that old code will suddenly start reading the wrong thing from source tables: {{getPsfFlux}} will simply not exist, instead of returning the wrong thing.

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

            I'm less clear on whether I agree with the period of time where there is no getPsfFlux_flux. I think it will be hard to logically define what time that absence should span. And then those columns will still be accessible through name/string-based access methods.

            I think the difference between getting the instrumental counts and the counts in physical units should be so obvious that it should break tests in code. I think people would rather get crazy answers that they then catch than have the code crash.

            I would support there being a stage in the development of DM-10302 where on those branches there are no such getters/setters. This should provide the opportunity to run the system through our various CI and related tests, without creating a master (and thus a subsequent weekly) that will instantly break lots of later-stage code.

            wmwood-vasey Michael Wood-Vasey added a comment - I'm less clear on whether I agree with the period of time where there is no getPsfFlux_flux . I think it will be hard to logically define what time that absence should span. And then those columns will still be accessible through name/string-based access methods. I think the difference between getting the instrumental counts and the counts in physical units should be so obvious that it should break tests in code. I think people would rather get crazy answers that they then catch than have the code crash. I would support there being a stage in the development of DM-10302 where on those branches there are no such getters/setters. This should provide the opportunity to run the system through our various CI and related tests, without creating a master (and thus a subsequent weekly) that will instantly break lots of later-stage code.

            The intention of this RFC is simply to adjust the naming of existing functionality, not to add anything new.

            My understanding — correct me if I'm wrong — is that we currently do not provide getters and setters for physical/sky flux, not least because we don't generally store physical fluxes in our source tables. I'd have no objection to adding them in the future, but that goes beyond the scope of this work.

            swinbank John Swinbank added a comment - The intention of this RFC is simply to adjust the naming of existing functionality, not to add anything new. My understanding — correct me if I'm wrong — is that we currently do not provide getters and setters for physical/sky flux, not least because we don't generally store physical fluxes in our source tables. I'd have no objection to adding them in the future, but that goes beyond the scope of this work.
            rowen Russell Owen made changes -
            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 sky 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 initially do *not* add getters and setters for sky flux. We can add them later if we really need them. By not defining them at first we reduce the danger that old code will suddenly start reading the wrong thing from source tables: {{getPsfFlux}} will simply not exist, instead of returning the wrong thing.

            I have taken the liberty of adding as watchers those who weighed in on RFC-322
            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 initially do *not* add getters and setters for physical flux. We can add them later if we really need them. By not defining them at first we reduce the danger that old code will suddenly start reading the wrong thing from source tables: {{getPsfFlux}} will simply not exist, instead of returning the wrong thing.

            I have taken the liberty of adding as watchers those who weighed in on RFC-322
            rowen Russell Owen added a comment - - edited

            wmwood-vasey I changed "sky flux" to "physical flux" in the description, as per your suggestion (if I missed any feel free to edit it yourself). Regarding your other point: users will always be able to access physical flux (if it is available) by accessing the field directly. All I am suggesting is that we do not offer convenience accessor methods. Use `record.get(name_or_key)` instead of `record.getPsfFlux()`. In the long run I believe we'd like to try to get rid of the convenience accessors, so I am reluctant to add more. All I am proposing to do is rename the convenience accessors we already have to make sure users realize that they deal with instrument flux rather than physical flux.

            rowen Russell Owen added a comment - - edited wmwood-vasey I changed "sky flux" to "physical flux" in the description, as per your suggestion (if I missed any feel free to edit it yourself). Regarding your other point: users will always be able to access physical flux (if it is available) by accessing the field directly. All I am suggesting is that we do not offer convenience accessor methods. Use `record.get(name_or_key)` instead of `record.getPsfFlux()`. In the long run I believe we'd like to try to get rid of the convenience accessors, so I am reluctant to add more. All I am proposing to do is rename the convenience accessors we already have to make sure users realize that they deal with instrument flux rather than physical flux.

            rowen Thanks for taking the time to explain to me in more detail.

            If the long-run plan is to get ride of the convenience accessors then I agree that there's no need to add new ones here.

            wmwood-vasey Michael Wood-Vasey added a comment - rowen Thanks for taking the time to explain to me in more detail. If the long-run plan is to get ride of the convenience accessors then I agree that there's no need to add new ones here.

            If the long-run plan is to get ride of the convenience accessors...

            As of now, this is speculation rather than a plan.

            However, I repeat that adding accessors for physical flux is outside the scope of this RFC, regardless of future plans.

            swinbank John Swinbank added a comment - If the long-run plan is to get ride of the convenience accessors... As of now, this is speculation rather than a plan. However, I repeat that adding accessors for physical flux is outside the scope of this RFC, regardless of future plans.

            Sounds good. I was likely distracted by the keeping open the possibility in the RFC text that implied that that might be added.

            Perhaps it would be clearer to update:

            I further propose that we initially do not add getters and setters for physical flux. We can add them later if we really need them. By not defining them at first we reduce the danger that old code will suddenly start reading the wrong thing from source tables: getPsfFlux will simply not exist, instead of returning the wrong thing.

            to

            I further propose that we do not add getters and setters for physical flux. Uses in old code that does not get updated will simply fail.

            wmwood-vasey Michael Wood-Vasey added a comment - Sounds good. I was likely distracted by the keeping open the possibility in the RFC text that implied that that might be added. Perhaps it would be clearer to update: I further propose that we initially do not add getters and setters for physical flux. We can add them later if we really need them. By not defining them at first we reduce the danger that old code will suddenly start reading the wrong thing from source tables: getPsfFlux will simply not exist, instead of returning the wrong thing. to I further propose that we do not add getters and setters for physical flux. Uses in old code that does not get updated will simply fail.
            rowen Russell Owen made changes -
            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 initially do *not* add getters and setters for physical flux. We can add them later if we really need them. By not defining them at first we reduce the danger that old code will suddenly start reading the wrong thing from source tables: {{getPsfFlux}} will simply not exist, instead of returning the wrong thing.

            I have taken the liberty of adding as watchers those who weighed in on RFC-322
            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 find 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

            wmwood-vasey I have updated the text (slightly differently than you proposed, but I hope it is sufficiently clear).

            rowen Russell Owen added a comment - wmwood-vasey I have updated the text (slightly differently than you proposed, but I hope it is sufficiently clear).

            Great, thank you. I appreciate the indulgence.

            wmwood-vasey Michael Wood-Vasey added a comment - Great, thank you. I appreciate the indulgence.
            rowen Russell Owen made changes -
            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 find 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
            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
            Parejkoj John Parejko added a comment - - edited

            One further question related to this: base_CircularApertureFlux and base_GaussianFlux (and other similar slot names): do we change them all to blahInstFlux here? I believe that is the correct approach, but they are not defined in afw, but rather meas_base.

            The same actually goes for all of the slots and fields, in fact (beyond the getters/setters described here): a PSF instrument flux field will go from PsfFlux_flux to PsfInstFlux_instFlux.

            Parejkoj John Parejko added a comment - - edited One further question related to this: base_CircularApertureFlux and base_GaussianFlux (and other similar slot names): do we change them all to blahInstFlux here? I believe that is the correct approach, but they are not defined in afw, but rather meas_base. The same actually goes for all of the slots and fields, in fact (beyond the getters/setters described here): a PSF instrument flux field will go from PsfFlux_flux to PsfInstFlux_instFlux .
            ktl Kian-Tat Lim added a comment - - edited

            I'm confused about that last.  I thought RFC-322 was about changing BlahFlux_flux to BlahFlux_instFlux, not BlahInstFlux_instFlux, which seems unnecessary.

            (And furthermore, this RFC is only about getters and setters, not the underlying slot or field names.)

            ktl Kian-Tat Lim added a comment - - edited I'm confused about that last.  I thought RFC-322 was about changing BlahFlux_flux to BlahFlux_instFlux , not BlahInstFlux_instFlux , which seems unnecessary. (And furthermore, this RFC is only about getters and setters, not the underlying slot or field names.)
            jbosch Jim Bosch added a comment -

            base_CircularApertureFlux and base_GaussianFlux (and other similar slot names): do we change them all to blahInstFlux here

            Definitely.  I'm pretty ambivalent about most of this, but if we change some of these suffixes, we need to change them all.

            Of course, that applies to the field name, not the algorithm name.  In other words, we'd be going from base_PsfFlux_flux to base_PsfFlux_instFlux, not base_PsfInstFlux_instFlux.

            jbosch Jim Bosch added a comment - base_CircularApertureFlux  and  base_GaussianFlux  (and other similar slot names): do we change them  all  to  blahInstFlux  here Definitely.  I'm pretty ambivalent about most of this, but if we change some of these suffixes, we need to change them all. Of course, that applies to the field name, not the algorithm name.  In other words, we'd be going from base_PsfFlux_flux to base_PsfFlux_instFlux, not base_PsfInstFlux_instFlux.
            rowen Russell Owen added a comment - - edited

            I'm a bit confused and am inclined to extend the close date until we have time to think things through. I realize this RFC isn't directly about slot or field names, but I would like a coherent solution and Parejkoj plans to do the work (is doing the work) for both RFCs on the same ticket.

            If I understand this correctly we were planning to have slot_PsfFlux point to base_PsfFlux so we could have one flag that applied to both kinds of flux and their associated errors, e.g. we could ask for slot_PsfFlux_instFlux to get inst flux and slot_PsfFlux_flux to get physical flux and slot_PsfFlux_flag to get the flag. However, if slots have special accessors then what do we do? And the names for the accessors that I propose here don't match well.

            On the other hand, if we want to keep the two fluxes more separate then I would expect us to have slot_PsfInstFlux for instrument flux and slot_PsfFlux for physical flux and use our existing suffix _flux to get flux in both cases. Then we'd have a separate flag for both, which I doubt we want, but the connection to any accessor methods is more obvious.

            If we can't resolve this fairly quickly here I suggest we try to find time to chat next week. I'm not enough of an expert on slots to know what we ought to do.

            rowen Russell Owen added a comment - - edited I'm a bit confused and am inclined to extend the close date until we have time to think things through. I realize this RFC isn't directly about slot or field names, but I would like a coherent solution and Parejkoj plans to do the work (is doing the work) for both RFCs on the same ticket. If I understand this correctly we were planning to have slot_PsfFlux point to base_PsfFlux so we could have one flag that applied to both kinds of flux and their associated errors, e.g. we could ask for slot_PsfFlux_instFlux to get inst flux and slot_PsfFlux_flux to get physical flux and slot_PsfFlux_flag to get the flag. However, if slots have special accessors then what do we do? And the names for the accessors that I propose here don't match well. On the other hand, if we want to keep the two fluxes more separate then I would expect us to have slot_PsfInstFlux for instrument flux and slot_PsfFlux for physical flux and use our existing suffix _flux to get flux in both cases. Then we'd have a separate flag for both, which I doubt we want, but the connection to any accessor methods is more obvious. If we can't resolve this fairly quickly here I suggest we try to find time to chat next week. I'm not enough of an expert on slots to know what we ought to do.
            rowen Russell Owen made changes -
            Planned End 10/Aug/18 6:30 PM 17/Aug/18 6:30 PM
            jbosch Jim Bosch added a comment -

            We do not want any field names starting with anything like "base_PsfInstFlux" or "slot_PsfInstFlux", full stop.  Those are algorithm names, not flux field names; think of them as being equivalent to "base_PsfPhotometry" or "slot_PsfPhotometry".

            We do want names like "base_PsfFlux_instFlux" or "slot_PsfFlux_instFlux", and those should be coupled with slot getter names like "getPsfInstFlux()", which would delegate to the "slot_PsfFlux_instFlux" field.  Despite the unfortunate consequences of camel case naming rules, all of these are flux field names.

            jbosch Jim Bosch added a comment - We do not want any field names starting with anything like "base_PsfInstFlux" or "slot_PsfInstFlux", full stop.  Those are algorithm names, not flux field names; think of them as being equivalent to "base_PsfPhotometry" or "slot_PsfPhotometry". We do want names like "base_PsfFlux_instFlux" or "slot_PsfFlux_instFlux", and those should be coupled with slot getter names like "getPsfInstFlux()", which would delegate to the "slot_PsfFlux_instFlux" field.  Despite the unfortunate consequences of camel case naming rules, all of these are flux field names.

            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 made changes -
            Status Proposed [ 10805 ] Adopted [ 10806 ]
            rowen Russell Owen made changes -
            Planned End 17/Aug/18 6:30 PM 10/Aug/18 6:30 PM
            rowen Russell Owen made changes -
            Link This issue is triggering DM-10302 [ DM-10302 ]
            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 made changes -
            Assignee Russell Owen [ rowen ] John Parejko [ parejkoj ]
            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.
            Parejkoj John Parejko made changes -
            Link This issue is triggering DM-15796 [ DM-15796 ]
            Parejkoj John Parejko made changes -
            Resolution Done [ 10000 ]
            Status Adopted [ 10806 ] Implemented [ 11105 ]

            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.