DM-41642: Fix crashes when FLUX keyword is not set in header. #217
Conversation
There was a problem hiding this comment.
I suppose this is ok, but it potentially obscures a problem, namely that the flux keyword is getting set to None. My fix would have been to change the formatting in the log message to
self.log.warning("Only one exposure found at %s %s. Dropping exposure %d.",
self.config.matchExposuresType, expTime, exposures[0][1])
IMO, it would make sense for log messages to always use %s formatting for quantities the code is reading from elsewhere or perhaps always. Code shouldn't crash from log messaging. (Sorry, this review got posted as a comment because I reflexively hit ctrl-return, which is what I do in slack comments to get a newline).
| @@ -500,7 +500,14 @@ def arrangeFlatsByExpFlux(exposureList, exposureIdList, fluxKeyword): | |||
| assert len(exposureList) == len(exposureIdList), "Different lengths for exp. list and exp. ID lists" | |||
| for expRef, expId in zip(exposureList, exposureIdList): | |||
| # Get flux from header, assuming it is in the metadata. | |||
| expFlux = expRef.get().getMetadata()[fluxKeyword] | |||
| try: | |||
| expFlux = expRef.get().getMetadata()[fluxKeyword] | |||
There was a problem hiding this comment.
Is this a keyword written by some other task or something that comes from a raw header? Should we issue a log message if you think this should be there?
There was a problem hiding this comment.
This is logged as missing afterwords. I didn't want to add a log in here just to get things unblocked.
But it turns out the source of this problem is that one of the data taking controllers was giving corrupt header information and what got into the repo depended on the order of ingestion of the different detectors. Not great!
erykoff
force-pushed
the
tickets/DM-41642
branch
from
636f919
to
4345504
Compare
No description provided.