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

Replace AmpInfo table objects with a regular class

    Details

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

      Description

      In the 2014 CameraGeom redesign, we opted to use afw.table objects to represent amplifiers in camera geometry.  IIRC, the primary reason was that this would allow cameras to add custom per-amp fields (by adding to the schema) while still making the object that represents an amplifier a concrete, final object (which, among other things, substantially simplifies iteration and persistence).  It also gave us a natural format (FITS binary tables) for storing amplifier definitions on disk.

      In hindsight, this now looks like a mistake:

      • We never actually used that extensibility - while the YAML-based camera construction pattern used in obs_lsst does add some fields that the old, FITS+config-based construction does not, these new fields (which add some linearity information) are not camera-specific, and could usefully be added to the main schema.
      • We're moving to YAML for camera definition in large part because FITS binary tables are not human-readable or editable.
      • Using afw.table objects (along with pybind11 limitations in propagating constness to Python) forced us to punch a hole in the language-level enforcement of immutability for these objects, contributing to frequent misuse of these objects and substantial confusion about what various amplifier fields represent (I plan to submit another RFC to attack that subject more directly later this week).
      • In 2014, afw.table was a shiny new hammer that made lots of problems look like nails.  Today, while it does still solve some important problems, it's also become clear that its complexity represents a serious maintenance challenge and its overuse is causing dependency-ordering issues - so we generally win by minimizing its use.

      For these reasons, I'd like to replace the AmpInfo table objects with a simple struct-like class for amplifiers (not a true struct, as I think getters and setters are still valuable here), at least in the public in-memory interfaces for CameraGeom.  afw.table classes (and possibly the AmpInfo specializations) would continue to be used for I/O behind the scenes to allow us to continue to read on-disk FITS amplifier tables, but would become an implementation detail that could potentially be phased out in the future.

      This would constitute an API change that we wouldn't (reasonably) be able to completely handle with a deprecation period, but as we can add the exact getters that are used in 99% of AmpInfoRecord usage, disruption should be minimal.

       

        Attachments

          Issue Links

            Activity

            Hide
            lguy Leanne Guy added a comment -

            DESC use AmpInfo in one place in their code, Jim Chiang said that they can easily update their code if we make this change

            Show
            lguy Leanne Guy added a comment - DESC use AmpInfo in one place in their code, Jim Chiang said that they can easily update their code if we make this change
            Hide
            tjenness Tim Jenness added a comment -

            Jim Bosch what do you want to do with this RFC?

            Show
            tjenness Tim Jenness added a comment - Jim Bosch what do you want to do with this RFC?
            Hide
            jbosch Jim Bosch added a comment -

            I'm just going to adopt this with DM-18610 as the implementation ticket.  That will need another RFC (as promised earlier) before landing, and keeping this one Proposed until that one materializes is just a waste of everyone's time.

            Show
            jbosch Jim Bosch added a comment - I'm just going to adopt this with DM-18610 as the implementation ticket.  That will need another RFC (as promised earlier) before landing, and keeping this one Proposed until that one materializes is just a waste of everyone's time.
            Hide
            tjenness Tim Jenness added a comment -

            Jim Bosch, Christopher Waters, the triggered work for this RFC has been completed. Does this mean that this RFC has been implemented?

            Show
            tjenness Tim Jenness added a comment - Jim Bosch , Christopher Waters , the triggered work for this RFC has been completed. Does this mean that this RFC has been implemented?
            Hide
            czw Christopher Waters added a comment -

            Yes, this should be implemented now.  I'll mark it so.

            Show
            czw Christopher Waters added a comment - Yes, this should be implemented now.  I'll mark it so.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Planned End:

                  Summary Panel