DM-54558: add Detector to VisitImage#31
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #31 +/- ##
==========================================
- Coverage 70.02% 69.43% -0.59%
==========================================
Files 73 75 +2
Lines 7379 7800 +421
==========================================
+ Hits 5167 5416 +249
- Misses 2212 2384 +172 ☔ View full report in Codecov by Sentry. |
czwa
left a comment
czwa
left a comment
There was a problem hiding this comment.
Initial thoughts after reading through.
| "assembled position of the amplifier." | ||
| ), | ||
| ) | ||
| horizontal_overscan_bbox: Box = pydantic.Field( |
There was a problem hiding this comment.
If this new version can't have two names for the same object (as we have in afw), I'd strongly prefer if these were serial_overscan_bbox, parallel_overscan_bbox, and serial_prescan_bbox. A code search shows both are used about the same amount (serial has 21 hits; horizontal, 18).
There was a problem hiding this comment.
I can add properties for the others, and will switch to your suggestions as the source-of-truth attributes, since that's what will be stored when we write this to JSON (etc).
TallJimbo
changed the base branch from
tickets/DM-54572
to
u/jbosch/DM-54558-tmp-base
TallJimbo
force-pushed
the
tickets/DM-54558
branch
4 times, most recently
from
c758d11 to
c9c78a5
Compare
TallJimbo
force-pushed
the
tickets/DM-54558
branch
2 times, most recently
from
083dd48 to
576718a
Compare
TallJimbo
force-pushed
the
tickets/DM-54558
branch
from
115e33b to
f5cf37a
Compare
czwa
left a comment
czwa
left a comment
There was a problem hiding this comment.
Sorry for the delay, this took a bit for me to understand. I think I'm generally happy, with the comments partially me rubber ducking to sort out (but please do correct if I got something wrong).
One overall comment: the docstrings don't conform to the dev guide. Are the typing annotations automatically picked up so the documentation has all of that information? Apologies if I missed this in the dev guide.
| case "summary_stats": | ||
| return tree.summary_stats | ||
| case "detector": | ||
| return Detector.deserialize(tree.detector, archive) |
There was a problem hiding this comment.
Why like this and not tree.detector.deserialize(archive)?
ETA: I think I figured it out. tree.detector is not an lsst.images.camera.Detector, nor do we have an exposed DetectorSerializationModel, because it could be represented by a dict (or something similarly simple). So we need to call the deserialize class method. The things I was comparing to do have their own exposed SerializationModel, so they have those properties as part of tree. Please correct if I'm getting it completely wrong.
There was a problem hiding this comment.
Mostly right. But not exposing DetectorSerializationModel was an oversight (now fixed), and it's really just that making deserialize a classmethod of the in-memory type (i.e. what Detector does) is what I've been considering the default/standard approach. Up until now I had been thinking of the alternate forms (deserialize_x on a parent serialization model, or free functions, or deserialize instance method on the serialization model) as exceptions for when there was no single in-memory type under our control.
But I have been wondering whether making deserialize an instance method of the serialization model type instead would be a better default, and the way you read this code adds some weight to that idea. I'm not going to do that on this ticket, because I've got others in flight that would conflict, but I might go do that shortly after.
| psf=self._psf, | ||
| obs_info=self.obs_info, | ||
| summary_stats=self.summary_stats.model_copy(), | ||
| detector=self._detector.copy() if copy_detector else self._detector, |
There was a problem hiding this comment.
I'm sure this has a reason, but why would we not want a deep copy of the detector as well? Is the assumption that the detector never changes for a VisitImage? We probably do the same thing in afw, but I can't seem to find it.
There was a problem hiding this comment.
Yeah, the assumption is that the detector basically never changes, and since it's potentially carrying around a lot of coordinate transformations, I didn't want to assume it would be trivial to copy.
The question of what to do about this in afw is actually what led to the now-despised cameraGeom immutability - when an object is immutable, you ~never need to deep-copy it, because you know you can't break any other code (and they can't break you) via a shared reference. So I figured that if everything attached to an Exposure was immutable, we wouldn't have to wonder about what its deep-copy was doing, and life would be better. That worked out well for some types (e.g. PSFs) and not others (Detector), so in lsst.images I'm trying to find more of a balance based on expected usage.
| ), | ||
| ) | ||
| serial_overscan_bbox: Box = pydantic.Field( | ||
| description="Bounding box of the serial (horizon) overscan region in the raw image." |
| "gain": 1.51526, | ||
| "read_noise": 5.557, | ||
| "saturation": 130537.0, | ||
| "suspect_level": NaN, |
There was a problem hiding this comment.
Not sure if this is a real issue, or just github being fussy about json syntax.
There was a problem hiding this comment.
True JSON doesn't allow nan/inf in numbers at all, so various less-formal conventions have sprung up for how to do it anyway. This one is pretty common, but picky JSON linters will still complain about it.
This keeps it from breaking if we chose not to retain any raw geometry after trimmed assembly.
Prefer serial/parallel over horizontal/vertical, but add properties to support them all.
This is just a from_legacy conversion of the detector attached to the DP2 legacy visit_image.
TallJimbo
force-pushed
the
tickets/DM-54558
branch
from
f5cf37a to
59b7640
Compare
2 participants
Checklist
doc/changes