DM-18610: Add fields, limited mutability, and trim/assembly-state tracking to cameraGeom #487

czwa · 2019-09-18T21:21:51Z

Implement cameraGeom Builder() methods
Remove AmpInfoRecord/Table/Catalog

kfindeisen

Sorry for the delay, it took me a long time to understand the changes (particularly 87cfb2d). I have two big-picture concerns:

The code misuses inheritance a lot. While I'll grant that quick conversion of client code requires that X::Builder be queriable just like the X it will create, it's dangerous to make this fundamental to the builder interface and even more dangerous to do it by inheritance. With inheritance (or duck-typing), in many places you can replace an object with its builder (no need to call finish), which defeats the goal of minimizing mutability.
Please look into architectures based on composition instead, or simply provide duplicate methods. These methods can be removed later as the responsibility for altering existing objects becomes factored into the builders (e.g., it sounds like amplifiers only need to be transformed in very specific ways).
Detector is a fairly ill-behaved class because a Detector object depends on the Camera that contains it, including correlated state changes in either direction. This leads to very messy code where Camera's and Detector's builders need to transfer responsibility back and forth. While you've clearly gotten it to work, I worry about maintainability; it might be a lesser evil to roll back the changes to Detector, at least until it can be better decoupled from Camera.

include/lsst/afw/cameraGeom/Amplifier.h

kfindeisen · 2019-09-30T17:15:33Z

include/lsst/afw/cameraGeom/Amplifier.h

+     * Copy the Amplifier's fields into the given record.
+     *
+     * @param[in,out] record   Record to modify.
+     *                         `record.getSchema().contains(this->getRecordSchema())` must be true.


Do you mean @param[out]? If not, please clarify how record is used as input.

kfindeisen · 2019-09-30T17:20:27Z

include/lsst/afw/cameraGeom/Amplifier.h

+    //@}
+
+    //@{
+    /// # TODO! NEVER DOCUMENTED!


Jira issue?

@czwa , could you take care of this one? I'm hoping you might actually know enough about how this field is used in ISR to be able to document it now.

I've fixed these to be more useful and represent what they currently are.

include/lsst/afw/cameraGeom/Amplifier.h

python/lsst/afw/cameraGeom/cameraFactory.py

kfindeisen · 2019-10-04T19:47:51Z

python/lsst/afw/cameraGeom/cameraFactory.py

-    pitch = lsst.geom.Angle(detectorConfig.pitchDeg, lsst.geom.degrees)
-    roll = lsst.geom.Angle(detectorConfig.rollDeg, lsst.geom.degrees)
-    return Orientation(offset, refPos, yaw, pitch, roll)
+    return detectorConfig.getOrientation()


At this point you might as well just get rid of the function and call getOrientation directly. It's not in __all__, so all uses should™ be confined to this file.

I apparently already removed the reference to makeOrientation in favor of getOrientation, so I've removed this method.

python/lsst/afw/cameraGeom/transformMap.cc

kfindeisen · 2019-10-04T19:51:48Z

python/lsst/afw/cameraGeom/utils.py

@@ -219,18 +220,18 @@ def makeImageFromAmp(amp, imValue=None, imageFactory=afwImage.ImageU, markSize=1
        img.set(imValue)
    # Set the first pixel read to a different value
    markbbox = lsst.geom.Box2I()
-    if amp.getReadoutCorner() == 0:
+    if amp.getReadoutCorner() == afwCameraGeom.ReadoutCorner.LL:


kfindeisen · 2019-10-04T22:58:19Z

include/lsst/afw/cameraGeom/Detector.h

+ * classes.  A Detector must be created initially as part of a Camera
+ * (see Camera::Builder::add), but can then be modified either individually
+ * (see Detector::rebuild and Detector::PartialRebuilder) or as part of
+ * modifying the full Camera (see Detector::InCameraBuilder).


The name InCameraBuilder is a bit confusing, because it suggests that it modifies the camera as a side effect (instead, it appears to mostly be used by Camera::Builder to make a new camera). Consider renaming.

Is PartialRebuilder used for anything other than tests?

I don't like "InCameraBuilder", either, but I haven't been able to come up with anything I like better.

I've been wondering since I saw your overall review comments whether I can just drop PartialRebuilder (which would free up Detector::Builder as a replacement for Detector::InCameraBuilder - though of course we're discussing renaming all of them on another thread).

@czwa , have you found any need for PartialRebuilder in any downstream packages yet? My original use case is (vaguely) about how obs_lsst updates the Detector attached to raw Exposures on read, so if you've managed to make that code work without PartialRebuilder, it's probably safe to get rid of it.

obs_lsst does use a rebuild without an attached camera (in assembly.py), so I believe this is a PartialRebuilder. I don't explicitly choose, so if this code changes, I can check that continues to work and fix it if it doesn't.

WithTransformBuilder? Kind of unwieldy...

FixedTransformsBuilder? DetachedBuilder?

@czwa's statement that he doesn't know actually makes me less worried about this - while I'm sure he is using a PartialRebuilder, it sounds like the APIs don't really involve the name (at least in Python, or I imagine in C++ if making heavy use of auto).

TallJimbo

I've completed my first pass at addressing review comments for all but the last commit (which I'll leave to @czwa). Changes are on new commits I intend to rebase and squash before merge.

On @kfindeisen 's big-picture points:

The code misuses inheritance a lot. While I'll grant that quick conversion of client code requires that X::Builder be queriable just like the X it will create, it's dangerous to make this fundamental to the builder interface and even more dangerous to do it by inheritance. With inheritance (or duck-typing), in many places you can replace an object with its builder (no need to call finish), which defeats the goal of minimizing mutability.
Please look into architectures based on composition instead, or simply provide duplicate methods. These methods can be removed later as the responsibility for altering existing objects becomes factored into the builders (e.g., it sounds like amplifiers only need to be transformed in very specific ways).

There are use cases for the builders behaving like the things they build in at least some respects, so as long as they're not actually violating Liskov substitutability, I'd like to keep the inheritance. If we change anything about this, I think they best way to go would be to not call them builders, but to keep discussions threaded let's take that to this line comment.

Detector is a fairly ill-behaved class because a Detector object depends on the Camera that contains it, including correlated state changes in either direction. This leads to very messy code where Camera's and Detector's builders need to transfer responsibility back and forth. While you've clearly gotten it to work, I worry about maintainability; it might be a lesser evil to roll back the changes to Detector, at least until it can be better decoupled from Camera.

The relationship between Detector and Camera is preexisting and not something we can change easily; the details may be down to history, but we're pretty boxed in by use cases and established APIs (DM-14980 has a bit of that history, if you're curious). And I don't think backing out the Detector changes here is at all viable - they're too tied to everything else, and frankly it's been quite difficult to find a solution that closes the immutability loophole we have right now without being even more complex, so I'm not optimistic we'll be able to come back later and do substantially better.

What might be possible is removing Detector::PartialRebuilder, which would let us merge and simplify Detector::Builder and Detector::InCameraBuilder, but this depends on whether we've actually ended up using Detector::PartialRebuilder downstream. Again, let's take further discussion to a relevant line comment for better threading.

Note that I've redirected a few other line comments to the threads linked above as well.

include/lsst/afw/cameraGeom/Amplifier.h

TallJimbo · 2019-10-09T15:42:18Z

include/lsst/afw/cameraGeom/Amplifier.h

+    //@}
+
+    //@{
+    /// # TODO! NEVER DOCUMENTED!


@czwa , could you take care of this one? I'm hoping you might actually know enough about how this field is used in ISR to be able to document it now.

include/lsst/afw/cameraGeom/Amplifier.h

include/lsst/afw/cameraGeom/Detector.h

src/cameraGeom/Camera.cc

TallJimbo · 2019-10-11T13:22:20Z

src/cameraGeom/Camera.cc

+    }
+    // Make a single big TransformMap.
+    auto transformMap = TransformMap::make(getNativeCameraSys(), connections);
+    // Make actual Detector objects, giving each the full TransformMap.


I don't have a definite use case for a detector wanting a transform to another detector, but it's not completely implausible. More importantly, given that the Detector::Builders only have the connections that tie to their own detector and not the non-detector ones, I think it'd be at least as much work to construct a distinct TransformMap for each detector that has both its transforms and the non-detector transforms but not any of the other detector-specific transforms.

TallJimbo · 2019-10-11T13:51:03Z

src/cameraGeom/Detector.cc

+    // Given that the number of amplifiers is generally quite small (even LSST
+    // only has 16), we just do a linear search in the vector, as adding a map
+    // of some kind is definitely more storage and code complexity, without
+    // necessarily being any faster.


Added doc comment, and moved this comment (with some tweaks) to the Detector::_amplifiers data member.

TallJimbo · 2019-10-11T14:12:57Z

src/cameraGeom/Detector.cc

+namespace {
+
+template <typename Iter>
+Iter findConnection(Iter first, Iter last, CameraSys const & toSys) {


Yeah, but not enough to be worth doing anything about, I think, when the alternatives involve either adding a new header, some more friending, or a public method.

kfindeisen · 2019-10-14T20:58:04Z

src/cameraGeom/TransformMap.cc

+        // Deserialized connections should already be standardized, but be
+        // defensive anyway.
+        auto const referenceSys = getReferenceSys(connections);
+        connections = standardizeConnections(referenceSys, std::move(connections));


All the std::move are a bit unfortunate, I guess I imagined it would all fit on one line. 😞

Do we need to worry about what the referenceSys is? I think I remember you saying something about how PIXELS is the only right answer...

All the std::move are a bit unfortunate

My turn to say, "that's C++[11] for you," I guess. After playing around in Rust a bit (where move is actually the default, and that feels surprisingly natural), it's a real shame C++ didn't bite the bullet and add an operator for it - it's too fundamental to involve that many characters.

Do we need to worry about what the referenceSys is? I think I remember you saying something about how PIXELS is the only right answer...

PIXELS is the only right answer for the nativeSys option in the Python configuration for Detector. It's also true that the TransformMaps created by cameraGeom actually always use FOCAL_PLANE as the referenceSys, but TransformMap itself is better behaved than the classes that currently use it, and does not rely on that.

src/cameraGeom/Camera.cc

TallJimbo

Thanks for another careful review, @kfindeisen, especially considering that both this and the geom changes were enormous and structured in a way that made them difficult to look at.

Unfortunately, unlike with the geom changes, I feel like we may have brought you in too late here - the issues you brought up really make me wish I had the time to step back and re-do some of this, or that I'd thought more about trying to make inheritance relationships more natural earlier in the process. I simply don't have time to do that now, and I think we do really need (soon) many of the changes here that are now impossible to decouple from the more problematic ones. As I've argued, I think the problems that remain are likely to remain hypothetical rather than practical, and we've at least traded public/Python mutability loopholes for private C++ mutability loopholes. I think that makes it worthwhile to proceed without major additional changes on this ticket. But I want to make it clear that this is about expediency and the branch being "good enough", rather than objecting fundamentally to your arguments.

This may just be rationalization, but I also think it may make sense for the work consolidating the Python construction logic to proceed before taking another big-picture look at these classes. That may well remove or replace some boundary conditions, and I am confident it will at least shrink the code surface that depends on the Builder classes, making further changes easier. That's not a guarantee that we'll actually be able to return to these - C++ developer time is always in short supply - but I hope we will.

AmpInfoRecord et al remain, and will be removed in subsequent tickets. API changes (especially in Python) have been minimized, but could not be avoided entirely.

We've apparently always ignored the documentation that said these should only be the usable parts of those regions, and it's better to bring that documentation in line with reality than change current usage.

* Add AssemblyState enum for amplifiers to increase flexibility of hasRawInfo(). * Add getDataBBox() accessor to amplifiers to provide consistent BBox access, regardless of AssemblyState. * Add helpers to CameraConfig class to generate correctly typed values from input. * Simplify CameraFactory to use fromConfig() and new CameraConfig methods to construct detectors. * Add method to show transformMap connections (as there was previously no way to inspect this easily from python).

* Defer AssemblyState to DM-21277. * Remove unneeded methods that duplicate functionality. * Be consistent with TransformMap.getConnections naming.

* Add AssemblyState enum for amplifiers to increase flexibility of hasRawInfo(). * Add getDataBBox() accessor to amplifiers to provide consistent BBox access, regardless of AssemblyState. * Add helpers to CameraConfig class to generate correctly typed values from input. * Simplify CameraFactory to use fromConfig() and new CameraConfig methods to construct detectors. * Add method to show transformMap connections (as there was previously no way to inspect this easily from python).

czwa force-pushed the tickets/DM-18610 branch from b1e447a to 216eb90 Compare September 24, 2019 19:09

czwa requested a review from kfindeisen September 24, 2019 20:12

czwa force-pushed the tickets/DM-18610 branch from 216eb90 to d8bbf7c Compare October 2, 2019 19:17

kfindeisen requested changes Oct 4, 2019

View reviewed changes

TallJimbo reviewed Oct 11, 2019

View reviewed changes

kfindeisen reviewed Oct 14, 2019

View reviewed changes

src/cameraGeom/Camera.cc Outdated Show resolved Hide resolved

kfindeisen reviewed Oct 14, 2019

View reviewed changes

src/cameraGeom/Camera.cc Outdated Show resolved Hide resolved

kfindeisen approved these changes Oct 14, 2019

View reviewed changes

TallJimbo reviewed Oct 15, 2019

View reviewed changes

TallJimbo force-pushed the tickets/DM-18610 branch from e3be8ef to ef434b8 Compare October 16, 2019 16:35

czwa force-pushed the tickets/DM-18610 branch from 29da6ae to 53fe196 Compare October 23, 2019 19:21

TallJimbo and others added 18 commits October 25, 2019 12:02

Doc fix in afw.table FunctorKeys.

b94fcfc

Add, use new Amplifier class in CameraGeom.

62b8dfc

AmpInfoRecord et al remain, and will be removed in subsequent tickets. API changes (especially in Python) have been minimized, but could not be avoided entirely.

Remove AmpInfoRecord/Table/Catalog.

109f005

Split Amplifier into class hierarchy for mutability control.

4b0f163

Remove now-unnecessary CameraSys constructor.

4b3e59d

Add swap implementation for CameraSys.

9cd674f

Rewrite TransformMap internals, and drop its Builder class.

487c74a

Move most of DetectorCollection into a templated base class.

e9819bb

Remove unused cameraGeom.fitsUtils module.

20d905e

Split Camera and Detector into hierarchies for mutability control.

7559a98

Reimplement and simplify Camera:findDetectors.

480d3a7

Change definition of amplifier over/prescan regions.

b5b1a79

We've apparently always ignored the documentation that said these should only be the usable parts of those regions, and it's better to bring that documentation in line with reality than change current usage.

Clean up cameraGeom Python copyright headers.

ed2295a

Update docstrings for linearity, lists, and typos.

4691f96

Use properties for detector config helper methods.

0d8bde3

Simplify builder code changes.

d5ddd9e

* Defer AssemblyState to DM-21277. * Remove unneeded methods that duplicate functionality. * Be consistent with TransformMap.getConnections naming.

Split Camera and Detector into hierarchies for mutability control.

e77f2b9

czwa added 4 commits October 25, 2019 12:28

Removed unneeded comments.

46fc001

Make detectorConfig convenience methods properties.

bb58134

Squash rebase build problems.

82f031a

czwa force-pushed the tickets/DM-18610 branch from 53fe196 to 82f031a Compare October 25, 2019 19:11

czwa merged commit 82f031a into master Nov 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-18610: Add fields, limited mutability, and trim/assembly-state tracking to cameraGeom #487

DM-18610: Add fields, limited mutability, and trim/assembly-state tracking to cameraGeom #487

czwa commented Sep 18, 2019

kfindeisen left a comment •

edited

kfindeisen Sep 30, 2019

kfindeisen Sep 30, 2019

TallJimbo Oct 9, 2019

czwa Oct 11, 2019

kfindeisen Oct 4, 2019

czwa Oct 11, 2019

kfindeisen Oct 4, 2019

kfindeisen Oct 4, 2019

TallJimbo Oct 10, 2019

czwa Oct 11, 2019

kfindeisen Oct 14, 2019

TallJimbo Oct 15, 2019

TallJimbo left a comment

TallJimbo Oct 9, 2019

TallJimbo Oct 11, 2019

TallJimbo Oct 11, 2019

TallJimbo Oct 11, 2019

kfindeisen Oct 14, 2019 •

edited

TallJimbo Oct 15, 2019

TallJimbo left a comment

DM-18610: Add fields, limited mutability, and trim/assembly-state tracking to cameraGeom #487

DM-18610: Add fields, limited mutability, and trim/assembly-state tracking to cameraGeom #487

Conversation

czwa commented Sep 18, 2019

kfindeisen left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TallJimbo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfindeisen Oct 14, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TallJimbo left a comment

Choose a reason for hiding this comment

kfindeisen left a comment •

edited

kfindeisen Oct 14, 2019 •

edited