DM-21706: Add filter definitions to support BOT #251
Conversation
python/lsst/obs/lsst/_instrument.py
Outdated
| LSST_IMSIM_FILTER_DEFINITIONS, TS3_FILTER_DEFINITIONS, TS8_FILTER_DEFINITIONS, \ | ||
| COMCAM_FILTER_DEFINITIONS |
There was a problem hiding this comment.
We now use parens for import continuation rather than \ for some reason.
| logger = lsst.log.Log.getLogger("LsstCamMapper") | ||
| logger.warn('Unknown physical_filter "%s" for %s %s; replacing with "%s"', | ||
| obsInfo.physical_filter, | ||
| obsInfo.observation_id, obsInfo.detector_unique_name, |
There was a problem hiding this comment.
If the obsInfo.detector_unique_name is necessary here it means we somehow have different filters for detectors within a visit, which would be pretty crazy. I don't much care if you want to keep it for some reason though.
There was a problem hiding this comment.
I want the user to be able to find the offending file unambiguously. I agree that it won't matter modulo CCS bugs/data corruption
| @@ -44,7 +44,7 @@ def setUp(self): | |||
| ccdExposureId_bits = 52 | |||
| exposureIds = {'raw': 3019031900001029, | |||
| } | |||
| filters = {'raw': 'NONE', | |||
| filters = {'raw': 'UNKNOWN', | |||
There was a problem hiding this comment.
See OOB conversation about NONE to UNKNOWN for this whole file.
There was a problem hiding this comment.
Summary: there is no FILTER keyword, so UNKNOWN is more appropriate than NONE. I suppose I could rebase the file to add this fact to the commit comment, but is it worth it?
| @@ -44,7 +44,7 @@ def setUp(self): | |||
| ccdExposureId_bits = 52 | |||
| exposureIds = {'raw': 3019053000001000, | |||
| } | |||
| filters = {'raw': 'NONE', | |||
| filters = {'raw': 'UNKNOWN', | |||
There was a problem hiding this comment.
See OOB conversation about NONE to UNKNOWN for this whole file.
There was a problem hiding this comment.
The OOB chat was about what these mean. NONE should be used for "there was no filter in place" i.e. equivalent to EMPTY (NB this is different from BLANK or CLEAR which means clear glass, but of a non-blocking kind) whereas UNKNOWN means we don't have sufficient information to say what this is, so either there was no header key present, or it was present but it was an empty card, or it contained a string that we were unable to map, so the filter remains a mystery and could be anything in reality.
There was a problem hiding this comment.
The objection to these changes was therefore that, given the above, this was a semantic difference, as NONE and UNKNOWN are not equivalent. However, it turns out that this is actually correct, and was previously wrong, as there was no information present in the test file, I am told.
| @@ -414,7 +414,7 @@ def test_parsetask_lsstCam_translator(self): | |||
| testType='BIAS', | |||
| seqNum=1, | |||
| dayObs="2019-03-19", | |||
| filter='NONE', | |||
| filter='UNKNOWN', | |||
There was a problem hiding this comment.
See OOB conversation about NONE to UNKNOWN here.
python/lsst/obs/lsst/filters.py
Outdated
| addFilter(BOTFilters_dict, | ||
| abstract_filter=af, physical_filter=pf, lambdaEff=lambdaEff) |
There was a problem hiding this comment.
This fits on one line.
| BOTFilters = [ | ||
| FilterDefinition(abstract_filter="unknown", physical_filter="UNKNOWN", lambdaEff=0.0), | ||
| ] |
There was a problem hiding this comment.
This also fits on one line.
python/lsst/obs/lsst/filters.py
Outdated
| FilterDefinition(physical_filter="550CutOn", lambdaEff=0.0)] | ||
|
|
||
| TS3_FILTER_DEFINITIONS = FilterDefinitionCollection( | ||
| *LsstCamFilters0, |
There was a problem hiding this comment.
Is there a reason we want to add the nominal LSST filters to TS3?
There was a problem hiding this comment.
TS3 used the LSSTCam filters before I worked on the code, and as I don't know what's out in the wild it seemed safer to leave them.
python/lsst/obs/lsst/filters.py
Outdated
| FilterDefinition(physical_filter="550CutOn", lambdaEff=0.0)] | ||
|
|
||
| TS8_FILTER_DEFINITIONS = FilterDefinitionCollection( | ||
| *LsstCamFilters0, |
There was a problem hiding this comment.
Is there a reason we want to add the nominal LSST filters to TS8?
There was a problem hiding this comment.
See comment on TS3
python/lsst/obs/lsst/filters.py
Outdated
| if nd == "empty": | ||
| if abstract_filter == "empty": | ||
| af = "empty" | ||
| else: | ||
| af = f"{abstract_filter}" | ||
| elif abstract_filter == "empty": | ||
| pf = nd | ||
| af = f"{nd.replace('.', '_')}" | ||
| else: | ||
| af = f"{abstract_filter}~{nd.replace('.', '_')}" |
There was a problem hiding this comment.
I think I get what this block is doing, but boy did I have to stare at it for a minute. I wonder if it could be clearer... At least I think it deserves a comment on what it's trying to achieve.
There was a problem hiding this comment.
Comment added.
| obsInfo = ObservationInfo(exp.getMetadata()) | ||
| try: | ||
| filt = afwImage.Filter(obsInfo.physical_filter) | ||
| except pexExceptions.NotFoundError: |
There was a problem hiding this comment.
Please catch a LookupError here rather than trying to specifically catch pexExceptions. We are generally trying to avoid pex.exceptions in python unless it's critical that we be able to catch that variant.
There was a problem hiding this comment.
I looked at the exception's type and didn't know that it was a subclass of LookupError. Fixed.
| filter=False) | ||
|
|
||
| if filter: | ||
| obsInfo = ObservationInfo(exp.getMetadata()) |
There was a problem hiding this comment.
You can give this a hint by adding translator_class=self.translatorClass as a second argument.
There was a problem hiding this comment.
That wasted too much time... Specifying translator_class=self.translatorClass broke the unit tests. We weren't setting LatissMapper.translator = LatissTranslator.
There was a problem hiding this comment.
I'm very sorry about that. I don't know how I missed it from Latiss. I just checked imsim and ts8 and they do have it set.
RobertLuptonTheGood
force-pushed
the
tickets/DM-21706
branch
from
2b054e5
to
38de1cc
Compare
With all the ND filters in the second BOT filter wheel we have more than 128 filters, so would need an extra bit in nbit_filter. Instead, we only use the "base" of the filter name (discarding anything after the ~) when calculating coaddExposureId. This is OK because we won't ever be coadding data of this type; additionally we assert that the ID of any filter that appears in a coaddExposureId fits in the allocated bits, and arrange that the real LSST filters satisfy this condition.
RobertLuptonTheGood
force-pushed
the
tickets/DM-21706
branch
from
38de1cc
to
65dd889
Compare
Now with LATISS! #250 was just to discuss with Tim