Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DM-27178: Standardize aliases on Gen 2 Exposure get #336

Merged
merged 5 commits into from
Dec 17, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
96 changes: 81 additions & 15 deletions python/lsst/obs/base/cameraMapper.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import os
import re
import traceback
import warnings
import weakref

from astro_metadata_translator import fix_header
Expand Down Expand Up @@ -1028,36 +1029,101 @@ def _setCcdDetector(self, item, dataId, trimmed=True):
detector = self.camera[detectorName]
item.setDetector(detector)

@staticmethod
def _resolveFilters(definitions, idFilter, filterLabel):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to have this as idFilter=None, filterLabel=None, to make it clear what the defaults are?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's my experience that default values tend to lead to error-prone code, so I'd prefer not to use them unless there's a specific benefit.

"""Identify the filter(s) consistent with partial filter information.

Parameters
----------
definitions : `lsst.obs.base.FilterDefinitionCollection`
The filter definitions in which to search for filters.
idFilter : `str` or `None`
The filter information provided in a data ID.
filterLabel : `lsst.afw.image.FilterLabel` or `None`
The filter information provided by an exposure; may be incomplete.

Returns
-------
filters : `set` [`lsst.obs.base.FilterDefinition`]
The set of filters consistent with ``idFilter``
and ``filterLabel``.
"""
# Assume none of the filter constraints actually wrong/contradictory.
# Then taking the intersection of all constraints will give a unique
# result if one exists.
matches = set(definitions)
if idFilter is not None:
matches.intersection_update(definitions.findAll(idFilter))
if filterLabel is not None and filterLabel.hasPhysicalLabel():
matches.intersection_update(definitions.findAll(filterLabel.physicalLabel))
if filterLabel is not None and filterLabel.hasBandLabel():
matches.intersection_update(definitions.findAll(filterLabel.bandLabel))
return matches

def _setFilter(self, mapping, item, dataId):
"""Set the filter object in an Exposure. If the Exposure had a FILTER
keyword, this was already processed during load. But if it didn't,
use the filter from the registry.
"""Set the filter information in an Exposure.

The Exposure should already have had a filter loaded, but the reader
(in ``afw``) had to act on incomplete information. This method
cross-checks the filter against the data ID and the standard list
of filters.

Parameters
----------
mapping : `lsst.obs.base.Mapping`
Where to get the filter from.
Where to get the data ID filter from.
item : `lsst.afw.image.Exposure`
Exposure to set the filter in.
dataId : `dict`
Dataset identifier.
"""

if not (isinstance(item, afwImage.ExposureU) or isinstance(item, afwImage.ExposureI)
or isinstance(item, afwImage.ExposureF) or isinstance(item, afwImage.ExposureD)):
return

if item.getFilter().getId() != afwImage.Filter.UNKNOWN:
return

actualId = mapping.need(['filter'], dataId)
filterName = actualId['filter']
if self.filters is not None and filterName in self.filters:
filterName = self.filters[filterName]
try:
item.setFilter(afwImage.Filter(filterName))
except pexExcept.NotFoundError:
self.log.warn("Filter %s not defined. Set to UNKNOWN." % (filterName))
# getGen3Instrument returns class; need to construct it.
filterDefinitions = self.getGen3Instrument()().filterDefinitions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are there two sets of parentheses here? Also, I guess I don't understand the call from gen2 code into gen3 code here or how that works.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because getGen3Instrument() returns an Instrument class, and not an object (though all Instrument subclasses are required to be constructible with no arguments). No idea why it was designed that way.

As for the Gen 2/3 distinction, I think Instrument was intended to be a bridge between the two, so it's not a violation to use it in Gen 2 code. Though I'll admit I'm calling it mostly out of laziness (there is a way to get at the filter definitions without using Instrument, but it would require making an API change to CameraMapper and all its subclasses, and getGen3Instrument was already there).

except NotImplementedError:
filterDefinitions = None
itemFilter = item.getFilterLabel() # may be None
try:
idFilter = mapping.need(['filter'], dataId)['filter']
except dafPersist.NoResults:
idFilter = None

if filterDefinitions is not None:
definitions = self._resolveFilters(filterDefinitions, idFilter, itemFilter)
self.log.debug("Matching filters for id=%r and label=%r are %s.",
idFilter, itemFilter, definitions)
if len(definitions) == 1:
newLabel = list(definitions)[0].makeFilterLabel()
if newLabel != itemFilter:
item.setFilterLabel(newLabel)
elif definitions:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be len(definitions) > 1 to be explicit?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was told that's Unpythonic, and that if definitions is the preferred style?

self.log.warn("Multiple matches for filter %r with data ID %r.", itemFilter, idFilter)
# Can we at least add a band?
# Never expect multiple definitions with same physical filter.
bands = {d.band for d in definitions} # None counts as separate result!
if len(bands) == 1 and itemFilter is None:
band = list(bands)[0]
item.setFilterLabel(afwImage.FilterLabel(band=band))
else:
# Unknown filter, nothing to be done.
self.log.warn("Cannot reconcile filter %r with data ID %r.", itemFilter, idFilter)
else:
if itemFilter is None:
# Old Filter cleanup, without the benefit of FilterDefinition
if self.filters is not None and idFilter in self.filters:
idFilter = self.filters[idFilter]
try:
# TODO: remove in DM-27177; at that point may not be able
# to process IDs without FilterDefinition.
with warnings.catch_warnings():
warnings.filterwarnings("ignore", category=FutureWarning)
item.setFilter(afwImage.Filter(idFilter))
except pexExcept.NotFoundError:
self.log.warn("Filter %s not defined. Set to UNKNOWN.", idFilter)

def _standardizeExposure(self, mapping, item, dataId, filter=True,
trimmed=True, setVisitInfo=True):
Expand Down
30 changes: 30 additions & 0 deletions python/lsst/obs/base/filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,31 @@ def reset(cls):
lsst.afw.image.utils.resetFilters()
cls._defined = None

def findAll(self, name):
"""Return the FilterDefinitions that match a particular name.

This method makes no attempt to prioritize, e.g., band names over
physical filter names; any definition that makes *any* reference
to the name is returned.

Parameters
----------
name : `str`
The name to search for. May be any band, physical, or alias name.

Returns
-------
matches : `set` [`FilterDefinition`]
All FilterDefinitions containing ``name`` as one of their
filter names.
"""
matches = set()
for filter in self._filters:
if name == filter.physical_filter or name == filter.band or name == filter.afw_name \
or name in filter.alias:
matches.add(filter)
return matches


@dataclasses.dataclass(frozen=True)
class FilterDefinition:
Expand Down Expand Up @@ -191,3 +216,8 @@ def defineFilter(self):
lambdaMin=self.lambdaMin,
lambdaMax=self.lambdaMax,
alias=sorted(aliases))

def makeFilterLabel(self):
"""Create a complete FilterLabel for this filter.
"""
return lsst.afw.image.FilterLabel(band=self.band, physical=self.physical_filter)
121 changes: 121 additions & 0 deletions tests/test_cameraMapper.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@

import lsst.utils.tests
import lsst.geom as geom
import lsst.afw.image as afwImage
import lsst.afw.table as afwTable
import lsst.daf.persistence as dafPersist
import lsst.obs.base
Expand All @@ -43,6 +44,37 @@ def setup_module(module):
lsst.utils.tests.init()


class MinCam(lsst.obs.base.Instrument):
@property
def filterDefinitions(self):
return lsst.obs.base.FilterDefinitionCollection(
lsst.obs.base.FilterDefinition(physical_filter="u.MP9301", band="u", lambdaEff=374),
lsst.obs.base.FilterDefinition(physical_filter="g.MP9401", band="g", lambdaEff=487),
lsst.obs.base.FilterDefinition(physical_filter="r.MP9601", band="r", alias={"old-r"},
lambdaEff=628),
lsst.obs.base.FilterDefinition(physical_filter="i.MP9701", band="i", lambdaEff=778),
lsst.obs.base.FilterDefinition(physical_filter="z.MP9801", band="z", lambdaEff=1170),
# afw_name is so special-cased that only a real example will work
lsst.obs.base.FilterDefinition(physical_filter="HSC-R2", band="r", afw_name="r2", lambdaEff=623),
)

@classmethod
def getName(cls):
return "min"

def getCamera(self):
raise NotImplementedError()

def register(self, registry):
raise NotImplementedError()

def getRawFormatter(self, dataId):
raise NotImplementedError()

def makeDataIdTranslatorFactory(self):
raise NotImplementedError()


class MinMapper1(lsst.obs.base.CameraMapper):
packageName = 'larry'

Expand All @@ -66,6 +98,7 @@ def getPackageDir(cls):

class MinMapper2(lsst.obs.base.CameraMapper):
packageName = 'moe'
_gen3instrument = MinCam

# CalibRoot in policy
# needCalibRegistry
Expand Down Expand Up @@ -357,6 +390,94 @@ def testStandardize(self):
self.assertEqual(mapper.canStandardize("raw"), True)
self.assertEqual(mapper.canStandardize("notPresent"), False)

def testStandardizeFiltersFilterDefs(self):
testLabels = [
(None, None),
(afwImage.FilterLabel(band="r", physical="r.MP9601"),
afwImage.FilterLabel(band="r", physical="r.MP9601")),
(afwImage.FilterLabel(band="r"), afwImage.FilterLabel(band="r", physical="r.MP9601")),
(afwImage.FilterLabel(physical="r.MP9601"),
afwImage.FilterLabel(band="r", physical="r.MP9601")),
(afwImage.FilterLabel(band="r", physical="old-r"),
afwImage.FilterLabel(band="r", physical="r.MP9601")),
(afwImage.FilterLabel(physical="old-r"),
afwImage.FilterLabel(band="r", physical="r.MP9601")),
(afwImage.FilterLabel(physical="r2"), afwImage.FilterLabel(band="r", physical="HSC-R2")),
]
testIds = [{"visit": 12345, "ccd": 42, "filter": f} for f in {
"r", "r.MP9601", "old-r", "r2",
}]
testData = []
# Resolve special combinations where the expected output is different
for input, corrected in testLabels:
for dataId in testIds:
if input is None:
if dataId["filter"] == "r":
data = (input, dataId, afwImage.FilterLabel(band="r"))
elif dataId["filter"] == "r2":
data = (input, dataId, afwImage.FilterLabel(band="r", physical="HSC-R2"))
else:
data = (input, dataId, afwImage.FilterLabel(band="r", physical="r.MP9601"))
elif input == afwImage.FilterLabel(band="r"):
if dataId["filter"] == "r":
# There are two "r" filters, can't tell which
data = (input, dataId, input)
elif dataId["filter"] == "r2":
data = (input, dataId, afwImage.FilterLabel(band="r", physical="HSC-R2"))
elif corrected.physicalLabel == "HSC-R2" and dataId["filter"] in ("r.MP9601", "old-r"):
# Contradictory inputs, leave as-is
data = (input, dataId, input)
elif corrected.physicalLabel == "r.MP9601" and dataId["filter"] == "r2":
# Contradictory inputs, leave as-is
data = (input, dataId, input)
else:
data = (input, dataId, corrected)
testData.append(data)

mapper = MinMapper2(root=ROOT)
for label, dataId, corrected in testData:
exposure = afwImage.ExposureF()
exposure.setFilterLabel(label)
mapper._setFilter(mapper.exposures['raw'], exposure, dataId)
self.assertEqual(exposure.getFilterLabel(), corrected, msg=f"Started from {label} and {dataId}")

def testStandardizeFiltersFilterNoDefs(self):
testLabels = [
None,
afwImage.FilterLabel(band="r", physical="r.MP9601"),
afwImage.FilterLabel(band="r"),
afwImage.FilterLabel(physical="r.MP9601"),
afwImage.FilterLabel(band="r", physical="old-r"),
afwImage.FilterLabel(physical="old-r"),
afwImage.FilterLabel(physical="r2"),
]
testIds = [{"visit": 12345, "ccd": 42, "filter": f} for f in {
"r", "r.MP9601", "old-r", "r2",
}]
testData = []
# Resolve special combinations where the expected output is different
for input in testLabels:
for dataId in testIds:
if input is None:
# Can still get some filter info out of the Filter registry
if dataId["filter"] == "r2":
data = (input, dataId,
afwImage.FilterLabel(band="r", physical="HSC-R2"))
else:
# Data ID maps to filter(s) with aliases; can't
# unambiguously determine physical filter.
data = (input, dataId, afwImage.FilterLabel(band="r"))
else:
data = (input, dataId, input)
testData.append(data)

mapper = MinMapper1(root=ROOT)
for label, dataId, corrected in testData:
exposure = afwImage.ExposureF()
exposure.setFilterLabel(label)
mapper._setFilter(mapper.exposures['raw'], exposure, dataId)
self.assertEqual(exposure.getFilterLabel(), corrected, msg=f"Started from {label} and {dataId}")

def testCalib(self):
mapper = MinMapper2(root=ROOT)
loc = mapper.map("flat", {"visit": 787650, "ccd": 13}, write=True)
Expand Down
13 changes: 13 additions & 0 deletions tests/test_filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,13 @@ def test_reset(self):
self.assertEqual(lsst.afw.image.Filter('abc').getFilterProperty().getLambdaEff(), 321)
self.assertEqual(lsst.afw.image.Filter('def').getFilterProperty().getLambdaEff(), 654)

def test_findAll(self):
self.assertEqual(set(self.filters1.findAll('r')), set())
matches = self.filters1.findAll('abc')
self.assertEqual(len(matches), 1)
match = list(matches)[0]
self.assertEqual(match.physical_filter, 'abc')


class TestFilterDefinition(lsst.utils.tests.TestCase):
def setUp(self):
Expand Down Expand Up @@ -112,6 +119,8 @@ def test_physical_only(self):
filter = lsst.afw.image.Filter('physical')
self.assertEqual(filter.getName(), 'physical')
self.assertEqual([], sorted(filter.getAliases()))
self.assertEqual(self.physical_only.makeFilterLabel(),
lsst.afw.image.FilterLabel(physical='physical'))

def test_afw_name(self):
"""afw_name is the Filter name, physical_filter is an alias.
Expand All @@ -122,6 +131,8 @@ def test_afw_name(self):
self.assertEqual(filter.getName(), 'afw only')
self.assertEqual(filter_alias.getCanonicalName(), 'afw only')
self.assertEqual(['afw_name'], sorted(filter.getAliases()))
self.assertEqual(self.afw_name.makeFilterLabel(),
lsst.afw.image.FilterLabel(physical='afw_name'))

def test_abstract_only(self):
"""band is the Filter name, physical_filter is an alias.
Expand All @@ -132,6 +143,8 @@ def test_abstract_only(self):
self.assertEqual(filter.getName(), 'abstract only')
self.assertEqual(filter_alias.getCanonicalName(), 'abstract only')
self.assertEqual(['abstract'], sorted(filter.getAliases()))
self.assertEqual(self.abstract.makeFilterLabel(),
lsst.afw.image.FilterLabel(band='abstract only', physical='abstract'))


class MemoryTester(lsst.utils.tests.MemoryTestCase):
Expand Down