DM-27169: Use FilterLabel in Exposure/ExposureInfo #332
Conversation
kfindeisen
force-pushed
the
tickets/DM-27169
branch
from
12f5373
to
29af4b1
Compare
There was a problem hiding this comment.
The gen2 looks fine although I haven't really looked at it.
The gen3 code will need testing in ci_hsc_gen3 in multiprocessing mode and with composite disassembly enabled (something we don't do by default). I don't really understand why we want disassembly to create a filter and filterLabel component or why we want to be able to assemble a disassembled Exposure that has both filter and filterLabel defined.
| # obs_test does not have physical filters, so include a fallback | ||
| exposureFilter = exp.getFilterLabel() | ||
| filterName = exposureFilter.physicalLabel if exposureFilter.hasPhysicalLabel() \ | ||
| else exposureFilter.bandLabel |
There was a problem hiding this comment.
I'm torn on whether this should be an x=if y else z or if y:\nx=...\nelse:\n. Since it's long enough to need a new line anyway, I don't know that this is an improvement. I never quite know myself what to do in this kind of case.
There was a problem hiding this comment.
I prefer conditional expressions when I want to make it clear that something is getting assigned, and it's just a question of what. This is especially true in languages that can't enforce that a variable is set exactly once.
kfindeisen
force-pushed
the
tickets/DM-27169
branch
from
29af4b1
to
d18eae1
Compare
|
@timj, sorry to bother you again, but I'd appreciate it if you could take a look at the delegation-based code. This version passes |
kfindeisen
force-pushed
the
tickets/DM-27169
branch
from
d18eae1
to
c15b6a7
Compare
This change simplifies the component code by reducing special-casing. This code is backwards-compatible with old Exposure files. This change also updates tests/data/calexp.fits to use the current ExposureInfo format, to avoid spurious round-trip errors from the input and output version numbers not matching.
The Filter component remains for backward-compatibility (in parallel with Exposure::getFilter()). Both will be deprecated on DM-27170 and removed on DM-27177; keeping them in sync simplifies things for users.
The Filter component remains for backward-compatibility (in parallel with Exposure::getFilter()). Both will be deprecated on DM-27170 and removed on DM-27177; keeping them in sync simplifies things for users.
These tests specifically require the physical filter, but changes in afw mean that Filter.getName() only returns the physical filter if there are no other names for it.
kfindeisen
force-pushed
the
tickets/DM-27169
branch
from
c15b6a7
to
e3d7fdd
Compare
This PR adds support for a
filterLabelcomponent in both Gen 2 and Gen 3, and removes some special handling ofFilterthat is not necessary when writing theExposureInfoformat introduced on lsst/afw#551. As in lsst/daf_butler#446, thefilterLabelcomponent is a temporary backwards-compatibility measure, to be removed once we no longer need to also supportFilter.This PR also updates one of the raw-file tests to use
FilterLabel, because the backwards-compatibility code introduced in lsst/afw#551 cannot easily handle prior filter conventions for both raw exposures and calexps, and the calexp conventions are more widely used.