I'm not happy with punting the documentation question. Clearly adding the docstring is a first step, but what is Jonathan Sick's proposal for this when effort becomes is available?

It's not just a code question, it's a decision about how LSST interprets headers.

Longer answer to the doc question is that the default behavior is documented in a docstring, and also gets reflected in the higher-level documentation for the obs framework itself. See https://dmtn-030.lsst.io/#framework-topic-type

I'd need to spend more time studying this to actually write out a design of pages and their scopes for this specific domain, but this is the general doc architecture that will accommodate what you're asking for. Writing this in a docstring now will remind the writer to consider this later in the framework docs.

My objection to this proposal is as follows:

• The base class in question is supposed to provide a very minimal and predictable default behavior. Every obs package must subclass this class.
• I think it is possibly reasonable to set darkTime = DARKTIME if present, else NaN. DARKTIME is a standard FITS keyword so that behavior is simple and predictable.
• What I object to is darkTime = DARKTIME if present else EXPTIME if present else NaN. That is not good default behavior in a base class such as this because:
• Most observatories will know in advance where the dark time is coming from. DARKTIME is a common keyword but perhaps it will be elsewhere. The subclass can then program that. If It is never available, and the observatory wants to use EXPTIME instead, great. But the observatory should make that explicit in its own code.

I'm not even that keen to set darkTime at all in the base class because I don't think DARKTIME is sufficiently standardized to make that a reasonable default behavior. The default implementation only sets exposure date and exposure time.

We already expect every obs package to subclass this class. it is a one-liner to set darkTime=DARKTIME (or whatever other keyword the observatory uses). Please don't clutter the base class!

But: if you expect that some observatories really do want to use this logic: darkTime = DARKTIME if present, else EXPTIME else NaN, then add that as a method to the base class and call it from the subclass. It might be nice to generalize the method a bit, but I don't think there are many cases where a VisitInfo field should be set from one keyword if present, else another.

Note that this code does not typically change names or values in the metadata, but it does pop items as they are used (with one or two necessary exceptions, where a keyword is used in conjunction with multiple other keywords, so it is not safe to pop).

My first preference is not to set darkTime in the base class at all (and that is how I originally wrote the code). That leaves the default implementation more generally usable by most observatories. Whenever an expected keyword is not found it results in a warning log message, and if an observatory that did NOT have the DARKTIME keyword used the default implementation an unwanted warning would be logged.

I do NOT believe in cluttering up the base class in order to save a single line in each of a half dozen subclasses, if it means adding unwanted warnings to the logs.

If anyone actually thinks it is reasonable to handle data from a particular, known camera with a particular known data model by setting darkTime = DARKTIME if present else EXPTIME else NaN then that could be a convenience method. But I can't believe any observatory would need that. The only place it seems reasonable to me is some sort of generic code that does the best it can with arbitrary data. That's not what this class is intended for, but a subclass could be written that works that way.

I think I agree with Russell Owen here, though I don't see much objection to defaulting to DARKTIME only. That said, the FITS standard does not refer to DARKTIME at all (it's not a reserved word, nor a required word), which means we can't really assume anything about it.

Withdrawing this proposal.

Instead, I will change the code using the darktime to check for NAN, which will allow us to catch the kind of problems this was meant to solve.