DM-10765: Replace existing WCS classes with SkyWcs #42

r-owen · 2017-12-12T22:32:58Z

No description provided.

laurenam · 2018-01-08T17:39:52Z

tests/test_brighterFatter.py

+        kernelPickleString = kernel.getArray().dumps()
+        # kernel.getArray().dump(self.filename) triggers an "unclosed file" warning with numpy 1.13.1
+        with open(self.filename, 'wb') as f:
+            f.write(kernelPickleString)



Is there a reason you didn't just fix this as:

with open(self.filename, 'wb') as f: kernel.getArray().dump(f)

?

Also, typo in your commit message: "it turns out numpy..."

numpy.dump is only documented to accept file path string, not a file object.

I'll fix the commit message.

I'll add that I think your fix, or some variant of it, would work. I just don't like relying on undocumented features.

laurenam · 2018-01-08T22:55:10Z

python/lsst/ip/isr/isrTask.py

@@ -404,6 +410,8 @@ def run(self, ccdExposure, bias=None, linearizer=None, dark=None, flat=None, def
        \param[in] fringes -- a pipeBase.Struct with field fringes containing
                              exposure of fringe frame or list of fringe exposure
        \param[in] bfKernel -- kernel for brighter-fatter correction
+        \param[in] camera -- camera geometry, an lsst.afw.cameraGeom.Camera;
+            used by addDistortionModel


Spacing should line up as, e.g. fringes

laurenam · 2018-01-18T21:11:35Z

python/lsst/ip/isr/isrTask.py

+        if camera is None:
+            raise RuntimeError("camera is None")
+        detector = exposure.getDetector()
+        if wcs is None:


wcs --> detector
(you've already checked for the wcs)

Also, should you move the check for the camera to the list here:
03dc329#diff-3a9973baa2fe4526d09a95e405f90269R443
i.e.

if self.config.doAddDistortionModel and camera is None: raise RuntimeError("Must supply a camera if config.doAddDistortionModel True")

in order to bail out earlier?

Thanks for catching that bug. I'll add the test you suggest. I wish it didn't have to be in 3 places, but I think you are right.

laurenam · 2018-01-18T21:13:32Z

python/lsst/ip/isr/isrTask.py

@@ -789,6 +804,27 @@ def overscanCorrection(self, exposure, amp):
            collapseRej=self.config.overscanRej,
        )

+    def addDistortionModel(self, exposure, camera):
+        """!Update the WCS in exposure with a distortion model based on camera geometry


A tad more detail would be nice here (i.e. expand a bit on the "based on camera geometry" statement)

laurenam · 2018-01-18T21:14:18Z

tests/test_addDistortionModel.py

@@ -0,0 +1,124 @@
+#
+# LSST Data Management System
+# Copyright 2017 LSST Corporation.


2018 (darn slow reviewers...!)

laurenam · 2018-01-18T21:16:33Z

tests/test_addDistortionModel.py

+        isrConfig = IsrTask.ConfigClass()
+        for name in isrConfig:
+            if name.startswith("do"):
+                setattr(isrConfig, name, False)


Is it guaranteed that all boolean flags will always start with "do"?

I think it is guaranteed that all flags that control whether an operation is to be performed will start with do, and that is what matters here. The idea is to minimally process the data.

That may be an informal convention that we've adopted, but it's not "guaranteed". If you think it should be, you need to RFC it.

I don't think it matters in this case. If the test passes, great. If not, the test an easily be updated to add any new flags that control which stages get performed. In any case, the rule need only apply to IsrTask for this test, and self-consistency within one task is more likely that from task to task. It certainly doesn't rise to the level of an RFC.

laurenam · 2018-01-18T21:20:06Z

tests/test_addDistortionModel.py

+import lsst.afw.geom as afwGeom
+from lsst.afw.coord import IcrsCoord
+from lsst.afw.image import ExposureF, ExposureInfo, Calib, VisitInfo
+from lsst.afw.geom import makeCdMatrix, makeSkyWcs


I just seems silly to me to have these individual imports in addition to afwGeom. I will again state my preference for the import ... as ... style so that it is clear where the functions came from when they are used lower down in a file without having to look back up to the top (in addition to being consistent with a lot of existing code).

laurenam · 2018-01-18T21:20:47Z

tests/test_addDistortionModel.py

+        self.detector = DetectorWrapper().detector
+        self.crpix = afwGeom.Point2D(50, 100)
+        self.crval = IcrsCoord(36 * afwGeom.degrees, 71 * afwGeom.degrees)
+        scale = 1.0 * afwGeom.arcseconds


https://developer.lsst.io/coding/python_style_guide.html#binary-operators-should-be-surrounded-by-a-single-space-except-for

test_brighterFatter.py was warning about an unclosed file and this was real: it turns out numpy 1.13.1 (at least) has a bug whereby numpy.dump(path) does not close the file it opens.

and enable it by default. This adds a distortion model based on camera geometry (much as some tasks used to do). This is the logical place for it, and the only reason it was not done earlier is that DistortedTanWcs could not be peristed.

Python was warning about invalid backslash sequences for Doxygen commands. Use `@` instead of backslash

This is a separate commit from adding addDistortionModel because the doxygen command cleanup commit made rebasing too much work.

r-owen force-pushed the tickets/DM-10765 branch from 130b1e8 to ebdbbb5 Compare December 13, 2017 02:04

r-owen force-pushed the tickets/DM-10765 branch from ebdbbb5 to 83c2a69 Compare January 3, 2018 01:13

r-owen force-pushed the tickets/DM-10765 branch 2 times, most recently from dbfd1c5 to 1631b07 Compare January 17, 2018 18:27

laurenam reviewed Jan 18, 2018

View reviewed changes

r-owen force-pushed the tickets/DM-10765 branch 4 times, most recently from d36dec0 to 9306588 Compare January 22, 2018 16:51

r-owen force-pushed the tickets/DM-10765 branch from 9306588 to 0e48ede Compare January 30, 2018 01:48

r-owen added 8 commits February 13, 2018 11:47

Git ignore .cache and pytest_session.txt

e96d5c5

Fix a pep8 warning in isrTask.py

ac6a89a

Fix test warning

e4df7b5

test_brighterFatter.py was warning about an unclosed file and this was real: it turns out numpy 1.13.1 (at least) has a bug whereby numpy.dump(path) does not close the file it opens.

Simplify import of Polygon

66e379c

Update for afw.image.Wcs->afw.geom.SkyWcs

888ee41

Add IsrTask.addDistortionModel

db76e87

and enable it by default. This adds a distortion model based on camera geometry (much as some tasks used to do). This is the logical place for it, and the only reason it was not done earlier is that DistortedTanWcs could not be peristed.

Fix python warnings in two tasks

a273dd4

Python was warning about invalid backslash sequences for Doxygen commands. Use `@` instead of backslash

Improve docs for IsrTask.addDistortionModel

9646733

This is a separate commit from adding addDistortionModel because the doxygen command cleanup commit made rebasing too much work.

r-owen force-pushed the tickets/DM-10765 branch from 0e48ede to 9646733 Compare February 13, 2018 19:47

r-owen merged commit 9646733 into master Feb 16, 2018

ktlim deleted the tickets/DM-10765 branch August 25, 2018 06:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-10765: Replace existing WCS classes with SkyWcs #42

DM-10765: Replace existing WCS classes with SkyWcs #42

r-owen commented Dec 12, 2017

laurenam Jan 8, 2018

r-owen Jan 19, 2018

r-owen Jan 22, 2018

laurenam Jan 8, 2018

laurenam Jan 18, 2018

r-owen Jan 19, 2018

laurenam Jan 18, 2018

laurenam Jan 18, 2018

laurenam Jan 18, 2018

r-owen Jan 19, 2018

PaulPrice Jan 22, 2018

r-owen Jan 22, 2018

laurenam Jan 18, 2018

laurenam Jan 18, 2018

DM-10765: Replace existing WCS classes with SkyWcs #42

DM-10765: Replace existing WCS classes with SkyWcs #42

Conversation

r-owen commented Dec 12, 2017

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

Choose a reason for hiding this comment