DM-13417 cleanup error reporting and docstrings in cameraGeom.utils #338
Conversation
|
"Start cleaning up method" doesn't tell me anything useful. What method are you cleaning up? How are you cleaning up? Why are you cleaning up? |
python/lsst/afw/cameraGeom/utils.py
Outdated
| """!Make an Image of a Camera | ||
| """Make an Image of a Camera. | ||
|
|
||
| Put each ccd's image in the correct location and orientation on the focal |
There was a problem hiding this comment.
"ccd" --> "CCD"? Or maybe "detector"?
python/lsst/afw/cameraGeom/utils.py
Outdated
| @@ -706,8 +710,8 @@ def makeImageFromCamera(camera, detectorNameList=None, background=numpy.nan, buf | |||
| @param[in] bufferSize Size of border in binned pixels to make around the camera image | |||
| @param[in] imageSource Source to get ccd images. Must have a getCcdImage method | |||
| @param[in] imageFactory Type of image to build | |||
| @param[in] binSize bin factor | |||
| @return an image of the camera | |||
| @param[in] binSize Step size between samples per image. | |||
There was a problem hiding this comment.
I think this makes less sense than "bin factor".
python/lsst/afw/cameraGeom/utils.py
Outdated
| @param[in] binSize bin factor | ||
| @return an image of the camera | ||
| @param[in] binSize Step size between samples per image. | ||
| @return an afw.image.Image of the entire camera. |
There was a problem hiding this comment.
I thought you were numpydocifying?
python/lsst/afw/cameraGeom/utils.py
Outdated
|
|
||
| Returns | ||
| ------- | ||
| `lsst.afw.geom.SkyWcs` |
There was a problem hiding this comment.
You need to specify a name:
ampWcs : `lsst.afw.geom.SkyWcs`
| @@ -73,17 +82,29 @@ def prepareWcsData(wcs, amp, isTrimmed=True): | |||
|
|
|||
| def plotFocalPlane(camera, fieldSizeDeg_x=0, fieldSizeDeg_y=None, dx=0.1, dy=0.1, figsize=(10., 10.), | |||
There was a problem hiding this comment.
Why are the defaults for fieldSizeDeg_x and fieldSizeDeg_y different?
Why are there underscores in the names?
There was a problem hiding this comment.
I have no idea about either of those comments, I'm just bringing the documentation up to standard. Do you know why?
There was a problem hiding this comment.
- git blame reveals that @kfindeisen added the arguments (and the underscores)
- The
_ydefault isNoneso that it defaults to the value provided for_x; seems reasonable
There was a problem hiding this comment.
I did not add the arguments, I merely renamed them from pupil* to field*. I can trace them back as far as @SimonKrughoff in 2014, they may be even older.
In any case, I have no objections to these names being brought up to convention if somebody feels it's within scope.
python/lsst/afw/cameraGeom/utils.py
Outdated
| Source to get ccd images. Must have a `getCcdImage()` method. | ||
| imageFactory : `lsst.afw.image.Image` | ||
| Type of image to make | ||
| detectorNameList : `list` |
python/lsst/afw/cameraGeom/utils.py
Outdated
| Use all Detectors if None. | ||
| binSize : `int` | ||
| Step size between samples. | ||
| i.e. `np.linspace(0, bbox.getHeight(), bbox.getHeight()/binSize)` |
There was a problem hiding this comment.
The "i.e." ("that is") is saying how it's used, not what it is.
There was a problem hiding this comment.
Where do we stand here? Paul's saying that this is e.g. not i.e. (and I agree), but I don't think that this is a helpful doc string even then. The original doc for binSize in makeImageFromCcd was "Bin the image by this factor in both dimensions" which is implemented in ccdImage = afwMath.binImage(ccdImage, binSize) and I think that that's what we should be saying here.
There was a problem hiding this comment.
I'll go with that one.
There were at least 4 different definitions of the binSize parameter used in the various functions defined in this file. It would have helped if they were all self-consistent to start with.
python/lsst/afw/cameraGeom/utils.py
Outdated
| title : `str | ||
| Title in display | ||
| showWcs : `bool | ||
| whether to include a WCS in the display |
There was a problem hiding this comment.
"Include a Wcs in the display?"
python/lsst/afw/cameraGeom/utils.py
Outdated
|
|
||
| Returns | ||
| ------- | ||
| The mosaic image. |
python/lsst/afw/cameraGeom/utils.py
Outdated
| @@ -447,6 +448,8 @@ def _prepareImage(self, ccd, im, binSize, allowRotate=True): | |||
| def getCcdImage(self, ccd, imageFactory=afwImage.ImageF, binSize=1): | |||
| """Return an image of the specified ccd, and also the (possibly updated) ccd""" | |||
|
|
|||
| log = lsst.log.Log.getLogger("cameraGeom.utils.ButlerImage") | |||
There was a problem hiding this comment.
Add "afw" prefix?
Ditto below.
parejkoj
force-pushed
the
tickets/DM-13417
branch
from
a8c8fe5
to
9618e00
Compare
Cleanup, correct, and expand on existing docstrings, in addition to converting them from doxygen->numpydoc.
skip None images without swallowing exceptions
parejkoj
force-pushed
the
tickets/DM-13417
branch
from
9618e00
to
8d2ec18
Compare
No description provided.