DM-27178: Standardize aliases on Gen 2 Exposure get #336
Conversation
The Instrument will allow tests of more complex CameraMapper functionality.
There was a problem hiding this comment.
This might be out of scope for this ticket, but I am not quite sure how the alias system is going to continue into the Gen3 Future. Specifically, are short-hand terms like r2 essentially deprecated and we must use full names (HSC-R2, which thankfully isn't too verbose), or are these aliases going to live on?
| @@ -1028,36 +1029,100 @@ def _setCcdDetector(self, item, dataId, trimmed=True): | |||
| detector = self.camera[detectorName] | |||
| item.setDetector(detector) | |||
|
|
|||
| @staticmethod | |||
| def _resolveFilters(definitions, idFilter, filterLabel): | |||
There was a problem hiding this comment.
Would it be better to have this as idFilter=None, filterLabel=None, to make it clear what the defaults are?
There was a problem hiding this comment.
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.
| item.setFilter(afwImage.Filter(filterName)) | ||
| except pexExcept.NotFoundError: | ||
| self.log.warn("Filter %s not defined. Set to UNKNOWN." % (filterName)) | ||
| filterDefinitions = self.getGen3Instrument()().filterDefinitions |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
| newLabel = list(definitions)[0].makeFilterLabel() | ||
| if newLabel != itemFilter: | ||
| item.setFilterLabel(newLabel) | ||
| elif definitions: |
There was a problem hiding this comment.
Should this be len(definitions) > 1 to be explicit?
There was a problem hiding this comment.
I was told that's Unpythonic, and that if definitions is the preferred style?
Yes, aliases are going away -- Gen 3 can't handle them, and As for references to old names, in-code references will be rewritten to use either physical or band names. I think the case of |
The new standardizer uses FilterDefinitions as input, where possible, and uses the enhanced precision of FilterLabel to write exactly the desired filter.
Not having access to FilterDefinition is an edge case, but one that might persist for a while.
kfindeisen
force-pushed
the
tickets/DM-27178
branch
from
1fc2c32
to
a099a3b
Compare
This PR rewrites the Gen 2 filter-patching code to use
FilterLabelinstead of the less preciseFilter, using anyFilterDefinitionobjects as the source of truth for band and physical filter designations. Forobspackages that do not useFilterDefinition(or, equivalently,Instrument), it falls back to the old behavior.The new algorithm is intended to remove references to filter aliases from
Exposureobjects, and to disambiguate imperfectly converted old-styleFilters. It is most important for calexps, which previously stored only the band name.