Details
-
Type:
RFC
-
Status: Implemented
-
Resolution: Done
-
Component/s: DM
-
Labels:None
Description
While refactoring our FITS reading code as part of DM-15500, I noticed that we currently permit reading into images with pixel types that cannot represent all values of the on-disk type (for example, we allow an on-disk int64 image to be read in as a uint16 in-memory image). It's not clear to me from looking at the code whether this was intentional, and it seems a bit dangerous.
Instead, I propose we only support the following conversions:
- any integer or floating point -> any floating-point (potential loss of precision, but no overflow)
- any unsigned integer -> any larger signed integer
- any signed integer -> any not-smaller signed integer
- any unsigned integer -> any not-smaller unsigned integer
Please note that I am proposing this change just because avoiding overflow seems like the right thing to do; if there is a use case for the current behavior, I'm happy to retain it. I do see lots of tests in afw using conversions that could overflow, but I haven't yet seen a case where it looks anything other than incidental.
We could also go a bit further and prohibit conversions that would entail a loss of precision, but it's much easier for me to imagine valid use cases for those conversions.
Attachments
Issue Links
- is triggering
-
- Done
Activity
If someone writes image = ImageI("/path/to/int64image.fits"), what should happen? They're being explicit that they want an ImageI. Do we have an interface that will provide an image of the correct pixel type? I tend to think that people should be allowed to shoot themselves in the foot if they are doing so knowingly and there's a known Proper Way to do it.
Paul Price
added a comment - If someone writes image = ImageI("/path/to/int64image.fits") , what should happen? They're being explicit that they want an ImageI . Do we have an interface that will provide an image of the correct pixel type? I tend to think that people should be allowed to shoot themselves in the foot if they are doing so knowingly and there's a known Proper Way to do it. This came up in the process of adding a way to read an image into its on-disk pixel type (DM-15500) without knowing it advance; at present we don't have a way to do that.
It sounds like we should at least retain a way to read while narrowing the pixel type; I think the remaining question is whether the existing ImageI("/path/to/int64image.fits") approach is sufficiently explicit, or if we should require something like ImageI("/path/to/int64image.fits", unsafeConvert=True).
Sorry, I read this and didn't comment. I agree with Paul that an explicit request for a type should be possible, but it should be very rare and I would be OK with an option preserveType=True.
Otherwise, I'm OK with making this change, although I don't think it's especially important.
We should also be removing the larger image types. There's no need for ImageD (we'd have to change the type in Kernel). I suspect that there's no need for 64-bit images either (I used them as a sleazy way of merging Footprints at some point, but that was always just a hack). The changes proposed here would make these changes a little safer.
I'm going to adopt this with an API that requires an explicit opt-in for conversions that could overflow; that seems like a slight improvement, it's easy to make in the course of other work on DM-15500 (which is why I'm bothering with this at all now), and no one has objected to that course of action.
This seems like a good idea. I, too, worry a bit that somebody might actually want to downcast, but I doubt it. In LSST DM the only use case I know of for reading integer images are reading masks, when we know the type, and reading raw images in ISR reads, which quickly converts them to float and is not a memory concern if it reads a too-large integer type. I wonder if Robert Lupton or Paul Price have any use cases we are not thinking of.