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

Replace afw.image.Filter with global-free label-only classes

    XMLWordPrintable

    Details

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

      Description

      The Filter object we use to describe the bandpass associated with an image has always been used inconsistently, and its reliance on a global map of all known filters is now causing real trouble integrating the Gen3 middleware with more obs packages.  This is in part because Gen3 allows data from multiple instruments to exist in the same data repository, so we're actually running up against cases where we're trying to define the filters for multiple instruments in the same Python process (which I think was always impossible, or at least subject to very surprising and inconsistent behavior).  But it's perhaps more true that prior to Gen3 we mostly just accepted that inconsistent behavior as a given we had to work around, and adapted to whatever labels the Filter system produced in any particular context without any attempt to define a sane data model.

      We gathered some input on what we'd want in a replacement on RFC-541, and I've sketched out a proposal for replacement classes on this gist.  Most of the proposal is there, but I've got a few additional notes:

      • I've sketched out Python APIs to make them easier for most developers to parse, but I think these will have to be implemented in C++ so we can attach them to ExposureInfo.
      • One of the issues I identified in RFC-541 is not addressed here: how to generate consistent unique integer IDs that depend on filters.  I think that's a hard problem, and I'd like to try to assert (at least for now) that we don't actually need to solve it:
        • Unique IDs that mangle visit+detector datasets don't an integer from the filter, because a visit (for which we will need integer IDs for other reasons) implies a physical filter.
        • The unique IDs for Objects (or the outputs of the current multi-band coadd processing) do not depend on filter, because they are explicitly merged across all bands.  While we may have processing steps that generate intermediate outputs that are specific to a certain band, and we may find it convenient for debugging to generate IDs for those that mangle tract+patch+filter, we have no requirement that those IDs remain consistent across runs (only that they be de-mangle-able).
      • Those of you already familiar with the Gen3 data model for filters will note that this is fairly different; I think the Gen3 model needs to be expanded to look more like this, and I'm hoping to be able to do so without having a different dimension for each different type of filter.

       

        Attachments

          Issue Links

            Activity

            Hide
            krzys Krzysztof Findeisen added a comment - - edited

            So a single class would probably have all of these fields but permit some to be None, but that's still a lot simpler than the (more normalized) polymorphic hierarchy.

            I think tagged classes would be a greater evil than a class hierarchy – we already have to work around way too many possibly-null values in our code. Your example had only full_name, so I assumed that everything could be normalized to that form. But it sounds like you're saying that clients do need to be able to distinguish between different kinds of filters.

            Show
            krzys Krzysztof Findeisen added a comment - - edited So a single class would probably have all of these fields but permit some to be None, but that's still a lot simpler than the (more normalized) polymorphic hierarchy. I think tagged classes would be a greater evil than a class hierarchy – we already have to work around way too many possibly-null values in our code. Your example had only full_name , so I assumed that everything could be normalized to that form. But it sounds like you're saying that clients do need to be able to distinguish between different kinds of filters.
            Hide
            jbosch Jim Bosch added a comment -

            I think this has converged aside from the question of whether to make this a class hierarchy (the original proposal) or use a single struct that may have some optional attributes.  At this point I think it makes sense to just adopt the RFC and see how both of those approaches fit within the codebase on the implementation ticket, especially as the participants in that conversation have dwindled to just me and Krzysztof Findeisen.

            Show
            jbosch Jim Bosch added a comment - I think this has converged aside from the question of whether to make this a class hierarchy (the original proposal) or use a single struct that may have some optional attributes.  At this point I think it makes sense to just adopt the RFC and see how both of those approaches fit within the codebase on the implementation ticket, especially as the participants in that conversation have dwindled to just me and Krzysztof Findeisen .
            Hide
            krzys Krzysztof Findeisen added a comment -

            The new filter system will be implemented as described in RFC-730.

            Show
            krzys Krzysztof Findeisen added a comment - The new filter system will be implemented as described in RFC-730 .
            Hide
            krzys Krzysztof Findeisen added a comment -

            Hmm... I could swear I set the resolution to "Duplicate", yet it's been changed to "Done".

            Show
            krzys Krzysztof Findeisen added a comment - Hmm... I could swear I set the resolution to "Duplicate", yet it's been changed to "Done".
            Hide
            ktl Kian-Tat Lim added a comment -

            I think "Retired" implies "Done"; there's no distinction in resolution in the RFC project.

            Show
            ktl Kian-Tat Lim added a comment - I think "Retired" implies "Done"; there's no distinction in resolution in the RFC project.

              People

              Assignee:
              jbosch Jim Bosch
              Reporter:
              jbosch Jim Bosch
              Watchers:
              Ian Sullivan, Jim Bosch, John Parejko, John Swinbank, Kian-Tat Lim, Krzysztof Findeisen, Tim Jenness
              Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:
                Planned End: