Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-316

Port obs_cfht to new CameraGeom

    Details

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

      Description

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

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

        Attachments

          Activity

          Hide
          krughoff 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
          krughoff 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
          krughoff 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(-)

          Show
          krughoff 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
          abecker 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
          Show
          abecker 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
          krughoff 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

          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.

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

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

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

            People

            • Assignee:
              krughoff Simon Krughoff
              Reporter:
              krughoff Simon Krughoff
              Reviewers:
              Andrew Becker [X] (Inactive)
              Watchers:
              Andrew Becker [X] (Inactive), Mario Juric, Paul Price, Simon Krughoff
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Summary Panel