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