DM-31924: add more flexible system for getting data ID packers from Instruments #320
Conversation
TallJimbo
force-pushed
the
tickets/DM-31924
branch
2 times, most recently
from
819d063
to
bafb5cd
Compare
|
Build failures here are just due to it not seeing the |
There was a problem hiding this comment.
I'm a bit surprised that we don't have any instrument-specific implementations of this: what instruments do you expect would need something other than the default? As noted in your long comment, the logic here is rather complicated, so do you think we truly need the flexibility?
| self._n_observations = fixed.records["instrument"].exposure_max # type: ignore[union-attr] | ||
| else: | ||
| self._n_observations = fixed.records["instrument"].visit_max # type: ignore[union-attr] | ||
| self._maxBits = (self._n_observations * self._n_detectors - 1).bit_length() |
There was a problem hiding this comment.
Why _maxBits and not _max_bits for consistency with the others?
There was a problem hiding this comment.
Will change. I'm stuck with the maxBits property as an inherited public and pre-existing interface, but I did intend to be consistently snake_case with the internals and new interfaces here.
There was a problem hiding this comment.
Worth deprecating the maxBits property and adding a new max_bits property?
There was a problem hiding this comment.
No, my bar for an API change to modernize capitalization is really high, even when the expected level of disruption is low.
| visit_packers=[ | ||
| config.packer.apply(visit_data_id), | ||
| config.packer.apply(full_data_id), | ||
| config.packer.apply(instrument_data_id, is_exposure=False), |
There was a problem hiding this comment.
I must be misunderstanding something here, because I don't see how this works; there's no visit in the dataId to pack.
There was a problem hiding this comment.
Calling apply here just constructs the ObservationDimensionPacker instance. It isn't until pack is called that you need the full data ID.
I'm guessing this is confusing in part because you've just looked at the meas_base PR, where calling apply on a config field ends up both constructing a packer and calling pack on it. It all boils down to DimensionPacker being a per-instrument-and-dimensions or (or per-skymap-and-dimensions) thing, while IdGenerator is a per-data-ID thing.
There was a problem hiding this comment.
Might be worth a comment to that effect here? Or maybe I'm just confused by having reviewed all this.
There was a problem hiding this comment.
I'm a bit surprised that we don't have any instrument-specific implementations of this: what instruments do you expect would need something other than the default? As noted in your long comment, the logic here is rather complicated, so do you think we truly need the flexibility?
This is all very much focused on having LSSTCam, LATISS, ComCam, and some of the LSST simulators override the behavior. I'm just trying to keep this ticket limited to structural changes rather than behavioral changes. The ticket for that override is DM-38688.
| self._n_observations = fixed.records["instrument"].exposure_max # type: ignore[union-attr] | ||
| else: | ||
| self._n_observations = fixed.records["instrument"].visit_max # type: ignore[union-attr] | ||
| self._maxBits = (self._n_observations * self._n_detectors - 1).bit_length() |
There was a problem hiding this comment.
Will change. I'm stuck with the maxBits property as an inherited public and pre-existing interface, but I did intend to be consistently snake_case with the internals and new interfaces here.
|
|
||
| def _pack(self, dataId: DataCoordinate) -> int: | ||
| # Docstring inherited from DimensionPacker._pack | ||
| detector_id = cast(int, dataId["detector"]) |
There was a problem hiding this comment.
This is a static-typing only cast that does nothing at runtime; it's just a signal to MyPy that says "I know this is going to be an integer so don't complain to me about it maybe being something else"; daf_butler tells MyPy that data ID values can be str or int, here it needs to be told that detector implies int.
| visit_packers=[ | ||
| config.packer.apply(visit_data_id), | ||
| config.packer.apply(full_data_id), | ||
| config.packer.apply(instrument_data_id, is_exposure=False), |
There was a problem hiding this comment.
Calling apply here just constructs the ObservationDimensionPacker instance. It isn't until pack is called that you need the full data ID.
I'm guessing this is confusing in part because you've just looked at the meas_base PR, where calling apply on a config field ends up both constructing a packer and calling pack on it. It all boils down to DimensionPacker being a per-instrument-and-dimensions or (or per-skymap-and-dimensions) thing, while IdGenerator is a per-data-ID thing.
TallJimbo
force-pushed
the
tickets/DM-31924
branch
2 times, most recently
from
a3b6118
to
b7b10cc
Compare
TallJimbo
force-pushed
the
tickets/DM-31924
branch
from
b7b10cc
to
740a2f1
Compare
Checklist
doc/changes