DM-24285: fitsExposureFormatter fails to read "Exposure" entries correctly

[czwa]

fitsExposureFormatter now uses the afw.image.ExposureFitsReader methods to access the full exposure as well as subcomponents.

[timj]

Looks good but a couple of things need to be fixed. I'll leave the real review to @TallJimbo since he knows more about the afw bits.

[TallJimbo]

One comment that probably just needs to spawn a new ticket; Jenkins will tell us if it needs to be a blocker for this one. Other than that I'd just echo Tim's comments.

[TallJimbo]

For the image, mask, and variance components, this now might return a pixel type other than the one declared by the StorageClass when that is something more specific than just Exposure. I think that might be what the comment in the old code about "Exposure exposing the type of its image components" was getting at, and if so, that might require changes in afw to address (i.e. we might not have a machine way to discover that the image of an ExposureF is an ImageF, so we can't easily cast).

That would make this ticket a much bigger pain to fix, but it also may be a more minor problem than what this ticket already fixes, and hence might be a good trade if we open another ticket to deal with it later. The question is whether this breaks Jenkins.

[timj]

It's all fine if you are using Exposure and Image everywhere in your storage classes. Note though that you know the python type so can't you do a cast immediately on read if the type you read isn't the same as the pytype that you expected? Can you construct an ImageF from an ImageI?

[timj]

I'd use isinstance rather than checking that the classes are identical -- this will let you handle Image and ImageF.

[TallJimbo]

It's all fine if you are using Exposure and Image everywhere in your storage classes.

...and that's only fine if the PipelineTasks never expect the on-disk type to be coerced to a different one. I seem to recall this being the case in ISR for some instruments, though I may be mixing up image->exposure conversion with pixel type conversion (though even that may be a problem here).

[timj]

but can you cast an ImageI to an ImageF? If you can then there's no problem.

[RobertLuptonTheGood]

imageI.convertF()

[timj]

@czwa it seems like it should be easy to deal with @TallJimbo 's concerns by using the convertX methods. Are you going to do that on this ticket?

[TallJimbo]

Not that easy, unless you hard-code facts like "you want image.convertD() to convert Exposure.image to ExposureD.image but image.convertF() to convert Exposure.variance to ExposureD.variance". Those are pretty stable facts, but daf_butler doesn't seem like the right place to put them (which would be afw).

[timj]

Oh. You are talking about a very very different thing to what I thought you were talking about. I was talking about the fact that ExposureFitsReader doesn't know the requested type for a component and so in theory you can get an ImageF but you had requested an ImageI. We can definitely solve that because we know that the StorageClass wants an ImageI python type so we know that we have to call convertI on it. You seem to be talking about constructing Exposures with different types inside? By default we get the exposure of the type you ask for and fall back to ExposureFitsReader if you were generically asking for Exposure. I think I'm confused a bit about your real issue.

[TallJimbo]

@timj I was talking about the same thing you originally thought I was talking about, but my original wording was a bit confusing (hopefully the editing made it better). If we've already (effectively) duplicating the afw component types in the storage class definitions (which is obvious now that I think about it), then I agree this is not nearly as bad as I thought.

[czwa]

I was going to defer the typing issue to a new ticket, as I'm not entirely certain how this happens. Is this an introduced issue from this ticket? I thought the checking in FitsExposureFormatter.read() would raise if the storageClasses didn't match (and no component was requested).

I'm running Jenkins on this ticket branch, and would like to get it resolved so I can get back to the ISR/DECam/gen3 crosstalk issue that prompted the bugfix.

[timj]

The discussion relates directly to the new code in that previously you read an ExposureF and then returned by definition an ImageF. Now that we are using native type for reading the image component there is a risk that your storage class says you want ImageF but your readImage returns an ImageI. This will cause the butler to raise an exception. My point is that you already know how to fix this because you know that the pytype is ImageF and that ImageI is not an instance of ImageF. You can have a dict that maps pytype to convert method and if not isinstance you call that convert method and return the result.