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

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

    Details

    • Type: RFC
    • Status: Adopted
    • Resolution: Unresolved
    • Component/s: DM
    • Labels:
      None

      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

            I was referring to (and am not proposing any changes to) the afw::image::TransmissionCurve API. Mostly, I am proposing here that those APIs remain distinct, in that a Filter never has a or is a TransmissionCurve.

            It sounds like you mean something stronger, that a Filter not only does not contain a TransmissionCurve, but does not refer to one, either. If that's the case, and the relationship between filters and transmission curves is only accessible through the Butler, then the text:

            A Filter may be associated with transmission curve information, but never holds it.

            A PhysicalFilter is associated with a transmission curve

            is confusing and should be removed.

             

            A PhysicalFilter is associated with a piece of hardware that defines just one piece of the total throughput of a system... A PhotometricSystemFilter does not correspond to any piece of hardware

            This sounds to me like these classes all behave exactly the same. If a Filter is really just an identifier regardless of role, perhaps we only need a single class for it? This would address John Parejko's concern about names without forcing us to write lots of code in terms of passing and holding a FilterBase.

            Show
            krzys Krzysztof Findeisen added a comment - - edited I was referring to (and am not proposing any changes to) the afw::image::TransmissionCurve API. Mostly, I am proposing here that those APIs remain distinct, in that a Filter never has a or is a TransmissionCurve . It sounds like you mean something stronger, that a Filter not only does not contain a TransmissionCurve , but does not refer to one, either. If that's the case, and the relationship between filters and transmission curves is only accessible through the Butler, then the text: A Filter may be associated with transmission curve information, but never holds it. A PhysicalFilter is associated with a transmission curve is confusing and should be removed.   A PhysicalFilter is associated with a piece of hardware that defines just one piece of the total throughput of a system... A PhotometricSystemFilter does not correspond to any piece of hardware This sounds to me like these classes all behave exactly the same. If a Filter is really just an identifier regardless of role, perhaps we only need a single class for it? This would address John Parejko 's concern about names without forcing us to write lots of code in terms of passing and holding a FilterBase .
            Hide
            jbosch Jim Bosch added a comment -

            If a Filter is really just an identifier regardless of role, perhaps we only need a single class for it?

            I'm coming around to this idea.  I do think it's important that the identifier have some structure to it - we do have use cases for canonical_name vs. full_name, and I think instrument and system.  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.

            Show
            jbosch Jim Bosch added a comment - If a Filter is really just an identifier regardless of role, perhaps we only need a single class for it? I'm coming around to this idea.  I do think it's important that the identifier have some structure to it - we do have use cases for canonical_name vs. full_name, and I think instrument and system.  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.
            Hide
            tjenness Tim Jenness added a comment -

            Is this RFC waiting on anything further?

            Show
            tjenness Tim Jenness added a comment - Is this RFC waiting on anything further?
            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 .

              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:
                  Planned End:

                  Summary Panel