DM-5922: Rework camera geometry to use the replacement for XYTransform #87
Conversation
r-owen
force-pushed
the
tickets/DM-5922
branch
7 times, most recently
from
3332a6d
to
87928cb
Compare
| from lsst.pex.config import Config, Field, ListField | ||
| from lsst.obs.subaru import HscDistortion, DistortionPolynomial | ||
| __all__ = ["xyTransformRegistry"] | ||
| __all__ = ["transformRegistry"] |
There was a problem hiding this comment.
Why do we export something that we've imported? That seems dangerous.
| @@ -131,9 +136,11 @@ class HscDistortionConfig(Config): | |||
| 1.98549941035e-37, | |||
| -8.74305862185e-38, | |||
| ]) | |||
| skyToCcdOrder = Field(dtype=int, doc="Polynomial order for conversion from sky to CCD (x and y identical)", | |||
| skyToCcdOrder = Field(dtype=int, | |||
| doc="(ignored) Polynomial order for conversion from field angle to focal plane", | |||
There was a problem hiding this comment.
If these are ignored, can we just remove them?
There was a problem hiding this comment.
OK. As long as you don't mind. In the long run I'd much rather have usable coefficients instead of using an iterative inverse, but that's out of scope for this ticket.
|
|
||
| Paramaters | ||
| ---------- | ||
| order: int |
| ---------- | ||
| order: int | ||
| Polynomial order | ||
| xCoeffs, yCoeffs: list of float |
|
|
||
| Returns | ||
| ------- | ||
| Forward or inverse coefficients for astshim.PolyMap |
There was a problem hiding this comment.
coeffs : `numpy.ndarray`
Coefficients for `astshim.PolyMap`.
| @@ -61,8 +65,12 @@ def testDistortion(self): | |||
| self.fail("Cannot find saved data %r; set SAVE_DATA = True and run again to save new data" % | |||
| dataPath) | |||
|
|
|||
| fieldAngleToFocalPlaneTolerance = transformRegistry["hsc"].ConfigClass().tolerance | |||
There was a problem hiding this comment.
I think it would be better to put a hard number here rather than using a Config value which could change.
There was a problem hiding this comment.
The value I'm testing against is the value used to determine the quantity being tested -- the default value of tolerance in the config. Yes if somebody changes that default then the unit test might fail, but that change would have to be done in this package, so the person who changed the default will see the problem right away and be able to deal with it.
| @@ -1,9 +1,12 @@ | |||
| from __future__ import absolute_import, division, print_function | |||
|
|
|||
| __all__ = ["transformRegistry"] | |||
There was a problem hiding this comment.
Squash this part with the previous commit. (But I don't think we should be exporting transformRegistry.)
| @@ -1,91 +0,0 @@ | |||
| // -*- lsst-c++ -*- | |||
There was a problem hiding this comment.
Explain in the commit message why it's being removed.
| @@ -1,3 +1,4 @@ | |||
| from __future__ import absolute_import, division, print_function | |||
There was a problem hiding this comment.
Squash with previous commit that had the same goal.
| ####bkgd = afwMath.makeStatistics(mi, afwMath.MEDIAN, sctrl).getValue() | ||
| # ###sctrl = afwMath.StatisticsControl() | ||
| # ###sctrl.setAndMask(mi.getMask().getPlaneBitMask("DETECTED")) | ||
| # ###bkgd = afwMath.makeStatistics(mi, afwMath.MEDIAN, sctrl).getValue() |
There was a problem hiding this comment.
Just delete these lines.
There was a problem hiding this comment.
Great! Thank you for permission.
r-owen
force-pushed
the
tickets/DM-5922
branch
6 times, most recently
from
d23497f
to
268a587
Compare
Update the HSC distortion model to use an astshim PolyMap Change PUPIL to FIELD_ANGLE Use lsst.afw.geom.transformRegistry instead of xyTransformRegistry Tighten inverse tolerance in test_distortion because the new code does better than the old Update the UPS table to include numpy and astshim
Fix pep8 issues Remove initial #!/...
The old C++ HscDistortion model is no longer usable for camera geometry (since it is an XYTransform) and has been superseded.
r-owen
force-pushed
the
tickets/DM-5922
branch
from
268a587
to
1197436
Compare
Update the HSC distortion model to use an astshim PolyMap
Change PUPIL to FIELD_ANGLE
Use lsst.afw.geom.transformRegistry instead of xyTransformRegistry
Update the UPS table to include numpy and astshim