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

Implement afw.cameraGeom Builder() methods

    Details

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

      Description

      RFC-585 initially presented the the replacement of AmpInfoTable with cameraGeom.Amplifier.  As part of this rework, implemented on DM-18610, all Camera, Detector, and Amplifier objects have been made immutable, with only `getXYZ()` accessors.  To change the values of any of these objects, a Builder subclass is used to create a new object that implements the setXYZ() methods, which can then be finalized into a new immutable base object.

      The accessors from the AmpInfoTable records have been copied over to the new Amplifier class, so in most cases, API changes are trivial.  New accessors will be added in future tickets (DM-21274, DM-21275, DM-21276, DM-21277) in an attempt to clean up the camera construction process (DM-21279, which will ensure the same code is used for camera construction whether the definitions are stored as FITS or yaml).

      This ticket migrates all DM code that currently access or change cameraGeom objects to the new API, hopefully preventing any disruption.  This has caused enough code churn that I'd like to get this (and DM-18610) reviewed and merged before starting on tickets that will make use of this easier-to-use interface.

        Attachments

          Issue Links

            Activity

            Hide
            krzys Krzysztof Findeisen added a comment - - edited

            So it sounds like the use of builders, specifically, is the new element. I have no objections to using builders in C++; they're a much more comprehensible and extensible API than dozen-argument constructors. Are you saying they will also be used in Python (where keyword-based constructors are more Pythonic)?

            I suppose I would want to have a deprecation period for the Detector and Camera constructors, but I'll defer questions about whether it's worth the effort to people more familiar with the codebase.

            This change is hard to make strictly backwards compatible during a deprecation period, because we really don't want to duplicate all of the afw::table machinery just for a bunch of deprecated methods.

            Why would you need to do that? If you're replacing one class with another, just deprecate the old class.

            Show
            krzys Krzysztof Findeisen added a comment - - edited So it sounds like the use of builders, specifically, is the new element. I have no objections to using builders in C++; they're a much more comprehensible and extensible API than dozen-argument constructors. Are you saying they will also be used in Python (where keyword-based constructors are more Pythonic)? I suppose I would want to have a deprecation period for the Detector and Camera constructors, but I'll defer questions about whether it's worth the effort to people more familiar with the codebase. This change is hard to make strictly backwards compatible during a deprecation period, because we really don't want to duplicate all of the afw::table machinery just for a bunch of deprecated methods. Why would you need to do that? If you're replacing one class with another, just deprecate the old class.
            Hide
            jbosch Jim Bosch added a comment -

            Why would you need to do that? If you're replacing one class with another, just deprecate the old class.

            We're more importantly making it so Detector holds instances of the new class instead of instances of the old one, so full backwards compatibility would mean either making the new class behave just like the old class (i.e. replicating its afw::table APIs just to deprecate them) or having Detector actually hold instances of both that are somehow kept in sync (which I think is just not a good use of developer time).

            Detectors are really the only way one ever gets an amplifier object in the wild (as opposed to unit tests of the amplifier objects), so it's that container-element relationship that matters.

             

             

            Show
            jbosch Jim Bosch added a comment - Why would you need to do that? If you're replacing one class with another, just deprecate the old class. We're more importantly making it so Detector holds instances of the new class instead of instances of the old one, so full backwards compatibility would mean either making the new class behave just like the old class (i.e. replicating its afw::table APIs just to deprecate them) or having Detector actually hold instances of both that are somehow kept in sync (which I think is just not a good use of developer time). Detectors are really the only way one ever gets an amplifier object in the wild (as opposed to unit tests of the amplifier objects), so it's that container-element relationship that matters.    
            Hide
            tjenness Tim Jenness added a comment -

            This was discussed today at the DM-CCB. CCB do not feel the need to provide full backwards compatibility. Once this RFC is approved please include a triggered ticket that blocks the DM-18119 v19 release ticket to ensure that there is sufficient documentation on this change included in the release notes.

            Show
            tjenness Tim Jenness added a comment - This was discussed today at the DM-CCB. CCB do not feel the need to provide full backwards compatibility. Once this RFC is approved please include a triggered ticket that blocks the DM-18119 v19 release ticket to ensure that there is sufficient documentation on this change included in the release notes.
            Hide
            tjenness Tim Jenness added a comment -

            Christopher Waters are you intending to adopt this RFC? I see the work itself is almost ready to merge (and it can't merge until this RFC is adopted).

            Show
            tjenness Tim Jenness added a comment - Christopher Waters are you intending to adopt this RFC? I see the work itself is almost ready to merge (and it can't merge until this RFC is adopted).
            Hide
            czw Christopher Waters added a comment -

            I've updated this to be triggered by the ticket that will resolve the issue.  I believe that Jim Bosch and Krzysztof Findeisen have resolved their concerns with `afw` on that branch, so I will mark this as adopted while I attempt to merge the commits into something that builds.

            Show
            czw Christopher Waters added a comment - I've updated this to be triggered by the ticket that will resolve the issue.  I believe that Jim Bosch and Krzysztof Findeisen have resolved their concerns with `afw` on that branch, so I will mark this as adopted while I attempt to merge the commits into something that builds.

              People

              • Assignee:
                czw Christopher Waters
                Reporter:
                czw Christopher Waters
                Watchers:
                Christopher Waters, James Chiang, Jim Bosch, John Parejko, John Swinbank, Kian-Tat Lim, Krzysztof Findeisen, Leanne Guy, Merlin Fisher-Levine, Tim Jenness
              • Votes:
                0 Vote for this issue
                Watchers:
                10 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Planned End:

                  Summary Panel