Review for work done to get obs_cfht up and running. I expect that obs_cfht will need more work: e.g. radial distortion model, added configs for coadd, revamping of MegacamMapper.paf to look more like modern repos. This was the minimum necessary to get things going.

Ready for review.

afw: There is actually only one commit that needs to be reviewed. I'll cherry-pick that commit onto master once the review is complete.
The commit is: 297a2b

 afw]$ git diff HEAD^ HEAD

diff --git a/python/lsst/afw/cameraGeom/utils.py b/python/lsst/afw/cameraGeom/utils.py

index ac038ea..5229753 100644

--- a/python/lsst/afw/cameraGeom/utils.py

+++ b/python/lsst/afw/cameraGeom/utils.py

@@ -48,7 +48,7 @@ try:

 except NameError:

     display = False

-def prepareWcsData(wcs, amp):

+def prepareWcsData(wcs, amp, isTrimmed=True):

     """

     Put Wcs from an Amp image into CCD coordinates

     @param wcs: WCS object to modify in place

@@ -56,8 +56,11 @@ def prepareWcsData(wcs, amp):

     """

     if not amp.getHasRawInfo():

         raise RuntimeError("Cannot modify wcs without raw amp information")

-    ampDims = amp.getRawDataBBox().getDimensions()

-    wcs.flipImage(amp.getRawFlipX(), amp.getRawFlipY(), ampDims)

+    ampBox = amp.getRawDataBBox()

+    wcs.flipImage(amp.getRawFlipX(), amp.getRawFlipY(), ampBox.getDimensions())

+    #Shift WCS for trimming

+    wcs.shiftReferencePixel(-ampBox.getMinX(), -ampBox.getMinY())

+    #Account for shift of amp data in larger ccd matrix

     offset = amp.getRawXYOffset()

     wcs.shiftReferencePixel(offset.getX(), offset.getY())

obs_cfht:
I left of the binary files from the diff in the interest of space.

obs_cfht]$ git diff --stat master

 bin/genCameraRepository.py             |  267 +++++++++++

 bin/genDefectRegistry.py               |    8 +-

 config/colorterms.py                   |    6 +

 config/processCcd.py                   |    3 +

 megacam/CFHTDefects.list               |    2 +-

 megacam/Full_Megacam_geom.paf          |    4 +-

 megacam/camera/camera.py               |  773 ++++++++++++++++++++++++++++++++

 policy/MegacamMapper.paf               |  125 ++++--

 python/lsst/obs/cfht/colorterms.py     |   14 +

 python/lsst/obs/cfht/ingest.py         |    2 +-

 python/lsst/obs/cfht/megacamMapper.py  |   45 ++-

 tests/testButler.py                    |    4 +-

 ups/obs_cfht.table                     |    1 -

 50 files changed, 1260 insertions(+), 47 deletions(-)

pipe_tasks:

pipe_tasks]$ git diff --stat master

 python/lsst/pipe/tasks/ingest.py |   21 ++++++++++++++++++---

 1 files changed, 18 insertions(+), 3 deletions(-)

afw:

• why was isTrimmed added to the function call? It is not used.

obs_cfht:

• bin/genCameraRepository.py
  • There is a peculiar mix of hard-coded variables in the code.
    Globals: PIXELSIZE = 0.0135 #mm/pix
    In-line : camConfigPath = os.path.join(outputDir, "camera.py")
    camConfig.plateScale = 5.93 #arcsec/mm
    Do these require a Config?
  • Not all methods have docstrings

• megacam/Full_Megacam_geom.paf
  • Was datasec too large by 1 col the whole time?

• megacam/camera/camera.py
  • Is this somehow redundant with the .fits files in that same directory, and Full_Megacam_geom.paf? That is, is it possible for Full_Megacam_geom.paf and camera.py to get out of synch? If so I'd consider storing only 1 copy of this information, unless both versions are needed.

• python/lsst/obs/cfht/megacamMapper.py
  • Is pyfits the right tool to read the defects?

• tests/testButler.py
  • If I set the package up from scratch, it does not setup meas.algorithms, so you should add this as a dependency in the table.
  • After setting up meas_alg, I get the error: AssertionError: testdata_megacam is not setup
  • I don't think this should provide an exception if its not part of the package requirements. Perhaps a warning?
  • I don't see testdata_megacam in cgit, so I can't run this unit test. I'd make sure if runs from a "setup -r ." with and without testdata_megacam setup

pipe_tasks:

• No comments

afw:

• why was isTrimmed added to the function call? It is not used.

This was an oversight. Added handling of isTrimmed.

obs_cfht:

• bin/genCameraRepository.py
  • There is a peculiar mix of hard-coded variables in the code.
    Globals: PIXELSIZE = 0.0135 #mm/pix
    In-line : camConfigPath = os.path.join(outputDir, "camera.py")
    camConfig.plateScale = 5.93 #arcsec/mm
    Do these require a Config?

The purpose of this script is to generate the camera repository for the butler to read. I'm not too worried about hard coding things since this should only be run when the camera changes, and if that happens, things may need to be changed anyway. For example, it currently reads a PAF. That should certainly go away as soon as there is a better way to get the information. Since it will not ever be a task, I'm not sure it needs a config.

• • Not all methods have docstrings

Added doc strings

• megacam/Full_Megacam_geom.paf
  • Was datasec too large by 1 col the whole time?

That seems to be the case.

• megacam/camera/camera.py
  • Is this somehow redundant with the .fits files in that same directory, and Full_Megacam_geom.paf? That is, is it possible for Full_Megacam_geom.paf and camera.py to get out of synch? If so I'd consider storing only 1 copy of this information, unless both versions are needed.

camera.py contains much of the information in Full_Megacam_geom.paf and is constructed from it. I don't think they will get out of sync, but I also think we should get rid of the paf and use an alternative mechanism for inputting camera information. These would preferably be in some native format provided by the instrument team itself. The fits files are orthogonal to camera.py and contain all the Amp level information.

• python/lsst/obs/cfht/megacamMapper.py
  • Is pyfits the right tool to read the defects?

I'm not sure what the criticism is here. Is it that the defects should be stored as afwTables instead of ad hoc FITS binary tables? If it is that, I'm just using what was in place before. I think the entire calibration handling including camera geometry and defects needs to be overhauled as keeping these in the obs_* packages is far too cumbersome. I was going to keep using the current system and change it if/when the defects were moved into a calibration repository.

• tests/testButler.py
  • If I set the package up from scratch, it does not setup meas.algorithms, so you should add this as a dependency in the table.

Added to ups table file.

• • After setting up meas_alg, I get the error: AssertionError: testdata_megacam is not setup
  • I don't think this should provide an exception if its not part of the package requirements. Perhaps a warning?
  • I don't see testdata_megacam in cgit, so I can't run this unit test. I'd make sure if runs from a "setup -r ." with and without testdata_megacam setup.

I changed the assertion to a warning. I'm a little concerned that the test will pass without data to work on, but I don't know a better solution. I made testdata_cfht a setupOptional. To the last bullet. That's a typo. The error should read testdata_cfht. I fixed that.

Everything looks good, except I'd like some warning to the user, preferably in-file, about how camera.py is generated from Full_Megacam_geom.paf, where edits should occur, and what the process is going from one to the other. It sounds like this is intended to be a temporary situation that we have both files versioned in the same repository, but I'd still like to avoid confusion until it gets resolved. Otherwise good to go.

I added a header to the the camera config file warning against editing. Merged and pushed.