Sketch of FilterDefinition and some examples

FilterDefinitionSketch.py

class FilterDefinitionCollection(collections.abc.Sequence):
    """An order-preserving collection of multiple `FilterDefinition`.

    Parameters
    ----------
    filters : `~collections.abc.Sequence`
        The filters in this collection.
    instrument : `str` (optional)
        An instrument name to prepend to the physical filter names.
        Leave `None` if the physical filter names already include the
        instrument name (e.g. "HSC-R").
    """
    def __init__(self, *filters, instrument=None):
        if instrument is not None:
            self._filters = []
            for filter in filters:
                new_filter = FilterDefintion(f"{instrument}-{filter.name}", abstract=filter.abstract, doc=filter.doc)
                self._filters.append(new_filter)
        else:
            self._filters = list(filters)
 
    def __getitem__(self, key): 
        return self._filters[key] 
 
    def __len__(self): 
        return len(self._filters) 
 
    def __repr__(self): 
        return "FilterDefinitions(" + ', '.join(str(f) for f in self._filters) + ')'


@dataclasses.dataclass(frozen=True)
class FilterDefinition:
    physical: str

    # NOTE: Should this be a frozenset() from the start? Or are the Instrument's filter definitions always one-one?
    abstract: str = None
    # NOTE: I don't have an immediate use for this, other than providing
    # useful information for those trying to understand what's in a Collection.
    # It could also be handled by inline comments in the FilterDefinitionCollection, but that seems less reliable?
    # Providing `doc` at least gives people a place to write notes?
    doc: str = None
    """Additional reference information about this filter."""

    def __str__(self):
        txt = f"FilterDefinition(physical='{self.physical}'"
        if self.abstract is not None:
            txt += f", abstract='{self.abstract}'"
        return txt + ")"

    @property
    def name(self):
        return physical

    # NOTE: No more `defineFilter()` calls due to afw:
    # there is no need for it, because filters are either unique (physical w/instrument) or generic (abstract).
    # Gen3 may want some kind of `defineFilter` functionality to create the physical/abstract mappings?
    # Also, if we ensure Instrument is included in the physical filter name,
    # and only call `afw.defineFilter()` on physical, then we don't have to
    # remove the singleton at all, probably?
    #def defineFilter(self):
    #    lsst.afw.image.utils.defineFilter(self.name, abstract=self.abstract)


# Examples

HSC_FILTER_DEFINITIONS = FilterDefinitionCollection(
    FilterDefintion("HSC-G", abstract="g"),
    FilterDefintion("HSC-R", abstract="r"),
    FilterDefintion("HSC-R2", abstract="r"),
    FilterDefintion("HSC-NB0387", abstract="N387"),
    # These two will require header remapping in astro_metadata_translator, at minimum to prepend "HSC-"
    FilterDefintion("HSC-UNRECOGNISED", doc="filter unknown during observation"),
    FilterDefintion("HSC-None", abstract="None", doc="No filter in use"),
)

# These names will require header remapping in astro_metadata_translator, to trim off the numbers,
# but these names are easier to work with.
DECAM_FILTER_DEFINITIONS = FilterDefinitionCollection(
    FilterDefintion("u DECam", abstract="u"),
    FilterDefintion("g DECam", abstract="g"),
    FilterDefintion("solid plate DECam"),
    FilterDefintion("VR DECam", abstract="VR", doc="very wide visible filter"),
)

# NOTE: I'm not at all sold on the "instrument" here:
# it might be easier to just name the LSST physical filters e.g. "LSST-g"...
# Either way, we'll likely need to remap the filter names from LSST headers to match these
# (or ensure the Camera team is outputting these names).
LSST_FILTER_DEFINITIONS = FilterDefinitionCollection(
    FilterDefintion("u", abstract="u"),
    FilterDefintion("g", abstract="g"),
    FilterDefintion("r", abstract="r"),
    FilterDefintion("i", abstract="i"),
    FilterDefintion("z", abstract="z"),
    FilterDefintion("y", abstract="y"),
    FilterDefintion("None", doc="No filter in use"),
    instrument="LSST"
)

GitHub doesn't seem to support line comments in gists, so you get all the heckling at once:

• Why do you need FilterDefinitionCollection instead of just list? Is this for consistency with the existing obs_base code?
• If the purpose of FilterDefinitionCollection is the instrument-name transformation, I suggest making that explicit (e.g., a list comprehension). Otherwise, people who look at a call like LSST_FILTER_DEFINITIONS will have to read the code to figure out what the actual filter names are.
• Why does FilterDefinitionCollection care about order? (also, "order-preserving-collection"? Why not call a sequence a sequence?)
• Why do you need two separate filter classes (FilterLabel vs. FilterDefinition)? If I understand this code right, FilterLabel isn't even used.
• Why does FilterDefinitionCollection.__init__ take varargs instead of a sequence or a collection? That seems like it just provides room for user error.

Thanks for the feedback. Responding point by point:

• it's for consistency, and also to support collection.defineFilters() or other things to e.g. populate gen3 databases or check for consistency with pre-registered abstract names.
• I think I'm now leaning away from the instrument-name transformation bit: we'll just call the LSST filters "LSST-u" and be done with it. But I'm torn on that, because we don't actually have any "standardized" filter names, so I'm not sure how mad people will be if we do that.
• Order matters for id generation in the current system. I think that's now irrelevant though? Good point.
• FilterLabel supports filter names in Exposure/ExposureInfo, while this populates gen3 databases and other places that need relationships (and/or populates an instrument-based singleton list).
• I'm not sure I'm following your question: do you not like the *filters or the instrument=None? I think we can kill the latter. The former is what the current version of this uses in obs_base.

Yes, by varargs I meant *filters as opposed to filters.

I think my only comment here is that while I think the Sequence interface (particularly order-preservation and integer-index lookup) is probably something we'll want in a transition period to support various Gen2 data ID <-> integer mappings, I think it will ultimately be unnecessary, so we may want to instead make this just an Iterable (for the interface; a list backing is still fine) and add integer-index operations as named method with names and docs that discourage unnecessary use to make it more likely we'll be able to remove them someday.