DM-38486: Combined dark seems to not have exposure time

[czwa]

Populate a visitInfo for combined calibration.

[timj]

In theory, if we are propagating input headers to output and adjusting all the times properly, then ObservationInfo should still work (because it will have the "raw" headers understood by it) and then you could use the obsInfo to visitInfo routine from obs_base.

[czwa]

Checking this manually to see if the header in an existing calibration could be converted fails with a ValueError: None of the registered translation classes ['DECam', 'SuprimeCam', 'HSC', 'MegaPrime', 'SDSS'] understood this header. The TELESCOP parameter is possibly the problem, with LSST AuxTelescope found in the header not matching the Rubin Auxiliary Telescope that the LATISS translator in obs_lsst has.
Constructing the visitInfo manually seemed the right solution, as the exposure time is ill defined for some products: flats represent some product of throughputs that isn't really time dependent, so the use of 1.0s times there (and in the fringe which has a similar issue) is to supply a time that is safe against dividing by the exposure time. The dark should scale correctly, but that scale isn't propagated into the input metadata (although it is in the output metadata). 1.0s cannot be the correct times for the bias, which is why that is special cased to 0.0.

[timj]

ValueError

Okay. The problem here is that the metadata translator for LATISS hasn't been registered yet because nothing knows it has to import lsst.obs.lsst.translators. The only way this would be imported is if the code asked butler for the instrument class and imported that (using Instrument.fromName()) and then it would work. The advantage of the ObservationInfo approach is you get a fully populated combo visitInfo but I understand that this might not be workable (especially if you are stripping headers like EXPTIME out the combined header rather than updating it) [I hope you aren't leaving the EXPTIME header in there with a confusing value]

Also, we need to fix the LATISS headers so they have the right telescope name...

[timj]

I have many questions. I've also re-read combineHeaders and it confuses me a lot because it seems to be making life hard for itself in many places and then not being used anywhere. There are two places it's called and none of them use the return value.

[timj]

Suggested change

	instrumentLabel=inputVisitInfo.getInstrumentLabel())
	instrumentLabel=inputVisitInfo.instrumentLabel)

[timj]

How come the exposure time isn't the sum of all the dark exposure times? We could easily calculate that in combineHeaders since we already loop over visitInfo to store the exposure times in the header.

[czwa]

It's 1.0s as the scale factors used to combine the input darks are the dark_time values. This normalizes the combined dark to have unit exposure time, which is then scaled to match the dark_time of the exposures that will be corrected (in IsrTask).

[timj]

I'm a bit confused by this call because we don't do anything with the header this calculates.

I also just looked in combineHeaders and I wonder if we can make use of the first and last parameters to merge_headers -- those are designed to allow you to pick the first DATE-OBS you encounter and the last DATE-END you encounter such that the ObservationGroup code at the end might not be needed. Also we now use DATE-BEG in addition to DATE-OBS.

[czwa]

This does set the values in the combined calibration, possibly accidentally, as the header is pulled from the calibration exposure and updated. I've assumed this was a pointer-like operation, and the exposure is updated as the header object is updated.

I don't recall why this was originally returning the header, and that can likely be removed now. For the ObservationGroup code, I'm not sure that it's adding anything essential (in the cp_pipe/ISR sense) to the headers. I thought the merge_headers call was finding all the common headers and adding them, then there's the provenance information being added manually, and from the example headers I've been looking at, there's only a DATE-OBS field being added. The examples appear to be adding that field via the except block, using the calibDate. The comment on that field is wrong (and updated on this branch), but this is another keyword that isn't used downstream anywhere.

[timj]

Okay. It's all very confusing and seems to be working differently to what it's documented to do. There is the added confusion that it generates a merged header but then copies everything that got dropped back into the original header but that's exactly what the first mode option does in merge_headers anyhow, and if the first and last parameters are used it will automatically makes sure it picks up the first DATE-OBS and the last DATE-END. The way it's now written you don't want merged to exist at all because you are wanting to use the other header you got from the exposure and then copy everything back out of merged into the alias.

I think ObservationGroup is used because that can sort the headers into date order before it reads back the start time and end time (which it gets from header translation).

[timj]

Would be convenient if we didn't have to do this because combineHeaders returned some kind of combo VisitInfo as well as the modified header.

[czwa]

I've moved this into the combineHeaders call, as that has the VisitInfo list already available, to avoid the unnecessary extra get.

[timj]

This looks okay. Thanks.

Maybe change the documentation for combineHeaders to say that it will update the header in place for calib as well as returning it.