DM-5922: Rework camera geometry to use the replacement for XYTransform #89
Conversation
| @@ -53,8 +53,8 @@ class GaussianPsfFactory(Config): | |||
| check=isPositive, | |||
| ) | |||
| sizeFactor = Field( | |||
| doc="Kernel size as a factor of fwhm (dimensionless); " | |||
| + "size = sizeFactor * fwhm; ignored if size is not None", | |||
| doc = "Kernel size as a factor of fwhm (dimensionless); " + | |||
There was a problem hiding this comment.
I didn't think the spaces should be there for keyword args?
There was a problem hiding this comment.
Also, I don't think the space needs to be there at all does it?
There was a problem hiding this comment.
I believe spaces around = for keyword arguments are:
- encouraged for keyword arguments that are on their own line
- discouraged for other cases
There was a problem hiding this comment.
I checked, and @r-owen is correct according to the style guide. This makes me sad; I don't think whitespace around = should be affected at all by how we split lines. But I also don't have the energy to RFC a change.
r-owen
force-pushed
the
tickets/DM-5922
branch
3 times, most recently
from
68060ad
to
3b5e7b3
Compare
| if detector is not None: | ||
| tanSys = detector.makeCameraSys(TAN_PIXELS) | ||
| pixToTanXYTransform = detector.getTransformMap().get(tanSys) | ||
| pixToTanPix = detector.getTransform(TAN_PIXELS) |
There was a problem hiding this comment.
Now that I'm seeing this interface in action in real code, I'm a bit bothered by how difficult it is to see what's going on. This case is saved by the helpful pixToTanPix variable name, but I think getTransform should be replaced by a function that really says what's going on:
- what other coordinate system is involved ("PIXELS")?
- are we going from the given cordinate system ("TAN_PIXELS") to the implicit one ("PIXELS"), or the reverse?
To keep the meaning of the method the same, I'd recommend calling it something like getTransformFromPixelsTo(target). It's a lot more verbose, but a lot more readable.
Of course, this is all pre-existing, but it seems like a good time to fix it.
There was a problem hiding this comment.
I agree. I'll make it detector.getTransform(fromSys, toSys). It's important to offer this, instead of using detector.getTransformMap().getTransform(fromSys, toSys) because, unlike TransformMap, Detector accepts camera system prefixes, defaulting to the current detector.
|
I fixed the primary complaint (which was well justified) and am much happier with the code now. |
r-owen
force-pushed
the
tickets/DM-5922
branch
from
41b5a65
to
2ba2574
Compare
No description provided.