It is exciting to see time-varying defects for a camera. Now if we can just get time-varying amplifier data...but that is outside the scope of this ticket.
The changes to ingestCalibs look sensible and well documented. However, I have a few minor requests:
The help for --calibType is as follows: ""If not None, use the input calibration type instead of finding it in fits header". None only occurs if the user omits the argument, and I find the help confusing. Please consider something like the following, instead: "Type of calibration image; if omitted, determine from the OBSTYPE header of the calibration image".
The following is slightly inefficient, since it creates a sequence from the pair of lists before iterating over it.
for thisDate, nextDate in zip(dates[:-1], dates[1:]):
This is only a concern if the lists of dates are long. Still, using itertools.izip would eliminate the worry. If you make that change, please also change the "else" clause, as well.
Please restore the blank line after the initial line of the doc string for resolveOverlaps. Python doc strings should have a one-line summary, followed by a blank line, followed by additional information.
I am a bit confused by some of these changes.
why do you only add "ccdnum" to self.mappings["raw"] if not ingesting to create a registry? I assume that process is harmed by knowing about ccdnum, but I am curious as to why, and it would be nice to explain this as a comment in the code.
The standard CamaraMapper version of _setTimes talks about changing a calib image, which is a "calexp", but your version talks about "the calib object in an raw Exposure". Several suggestions:
- Clarify image types are changed. Raw or calexp or...? If yours is different than the standard CameraMapper it would be nice to have an explanation in the doc string as to why that is so.
- Emphasize what is different about your override vs. the standard method.
- Change "in an raw" to "in a raw"
- Add a blank line after the first sentence
I don't quite understand how trimming works. You trim the input exposure, not the bias and flat images, which implies you trim the data when applying bias and flat, rather than the other way around, despite the help strings for the config params saying the opposite. So...
If bias and flat are smaller than raw data, then I suggest you compute the amount of trim required based on the dimensions of the provided data. Why have config fields for this? The results are more likely to be correct. As it stands, if you get the values wrong you'll get an exception, and if you can compute a result, surely that's safer and simpler? Especially since if you ever do get master flats and biases that are full sized the code will "just work" with no reconfiguration.
If bias and flat are the same size or later than raw data then I really don't understand the code, and it could use some additional documentation.
A few more code notes:
I suggest you replace this:
from lsst.ip.isr import IsrTask
from lsst.ip.isr import isr as lsstIsr
from lsst.ip.isr import IsrTask, biasCorrection, flatCorrection
and ditch all use of "lsstIsr" (which I found confusing, as I thought you might be importing lsstSim's version of bias and flat correction).
afwImage.PARENT is redundant in the following, and should probably be removed, because we (I, to be honest) went to great lengths to make the default PARENT and that default is used nearly everywhere:
If you do decide you need to keep the config fields, please consider removing the term "MasterCal" from the names. These are presumably the bias and flats we have, and it doesn't matter where they come from. I also suggest clarifying that these are used to trim the data down to the size of the bias and flat images. Finally, I suggest changing "on each edge in the..." to "on each edge of the..." in the help strings.
Please explain why you had to override the run method. The standard run method also updates the variance plane. Please put this explanation in the doc string for the method.
I think your use of SourceDetectionTask.setEdgeBits is reasonable. It is a pity that this function is not available some other way, since it is a common need, but refactoring that out into a free function is presumably out of scope for this ticket. You could file a ticket about that, if you are willing. Note that coaddition also needs to do this, though it has some extra needs.