Port obs_cfht to new CameraGeom

XMLWordPrintable

Details

• Type: Improvement
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
None
• Team:

Description

Port the current CFHT camera description to the new current CameraGeom framework.

As a first go, I'll translate the existing paf.

Activity

Hide
Simon Krughoff added a comment -

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.

Show
Simon Krughoff added a comment - 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.
Hide
Simon Krughoff added a comment -

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]$git diff --stat master  python/lsst/pipe/tasks/ingest.py | 21 ++++++++++++++++++---  1 files changed, 18 insertions(+), 3 deletions(-) Show Simon Krughoff added a comment - 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(-)
Hide
Andrew Becker [X] (Inactive) added a comment -

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

Show
Andrew Becker [X] (Inactive) added a comment - 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
Hide
Simon Krughoff added a comment -

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

• 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.

• 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.

Show
Hide
Andrew Becker [X] (Inactive) added a comment -

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.

Show
Andrew Becker [X] (Inactive) added a comment - 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.
Hide
Simon Krughoff added a comment -

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

Show
Simon Krughoff added a comment - I added a header to the the camera config file warning against editing. Merged and pushed.

People

Assignee:
Simon Krughoff
Reporter:
Simon Krughoff
Reviewers:
Andrew Becker [X] (Inactive)
Watchers:
Andrew Becker [X] (Inactive), Mario Juric, Paul Price, Simon Krughoff