A few minor improvements to scaling and uint support - #3916

Incidentally, this also fixes #3766. I'm just going to cherry pick that one change out as a separate fix.

Need to double check if this merits any documentation changes (including a changelog entry, of course) before merging.

Added a note in the docs about unsigned integers, as there really wasn't anything there before.

@embray -- what is the expected behavior of the BZERO keyword when uint=True?

If uint=False then the BZERO keyword is removed from the header after the data is accessed/scaled. That makes sense because the header is consistent with the state of the data.

With uint=True, the data is uint16, which makes sense, but the header still has BZERO=32768 (assuming BITPIX=16).

If that is the intended behavior, I can add something about it to the docs. If it is not the intended behavior, I can try to fix so that BZERO gets droped.

@mwcraig Thanks for the catch--that sounds like a regression to me.

@embray -- I see the same behaviour in 1.0.3 if I set update=True, so I don't think this PR introduced the error, it just exposed it because it changed the default.

Okay, so not technically a regression, but definitely not intentional either.

Actually, no, I stand corrected.

So it is intentional?

Sorry, it is intentional now that I think harder about it. The reason is that there is no native unsigned integer type in FITS. So when you read unsigned int data, although technically it is rescaled, it leaves BZERO in the header because "BITPIX = N" plus "BZERO = 2**(N-1)" is the only way to unambiguously write "the data currently in the .data attribute is unsigned integer data".

The difference with arbitrary BSCALE/BZERO is that the data is being rescaled from some storage values (perhaps detector counts or some other quantized data) to "physical values" via the scaling, and the implication is that unless otherwise requested the data will remain as the physical values (so as to not lose accuracy in case the data is modified in any way).

Unsigned ints on the other hand are a special case since they're using BZERO as a kludge to represent a data type that FITS doesn't already natively support :(

This should go in the docs somewhere, I guess. One day I still want a less confusing way to represent this.... (somewhere else I've mentioned wanting to make a cleaner distinction between the "logical" data (including headers) and the raw data of the FITS serialization....)

I see - so in the case when uint=False the keywords get removed not because the data have been scaled, per se, but because the data type is unambiguous (BITPIX is either 32 or 64 in that case)?

It seems like that makes the meaning of the data a little ambiguous though...how does one know whether it has already been scaled with BZERO?

It is remarkable that FITS can't represent something simple like this!

I'll poke through the docs for a place to add it. Is there a recommended way to know whether the data has been scaled?

So with uint=False you're turning off the support for this implicit convention--this is supported because in principle one could use BSCALE=1 and BZERO=32768 with no intention whatsoever for the data to be interpreted as unsigned ints; in practice this is highly unlikely, which is why I changed the default. There is no way with FITS to know, without a doubt, that the uint convention is being used. With uint=False the BZERO keyword is removed after scaling because the data is just assumed to be rescaled to floats, just as in the general case. (In both cases the reason the keywords are removed is so that what's in the .header is in sync with what's actually in the .data.)

Part of the point of this PR is that fits.info() tries to tell a little more about what's going on with data rescaling. It's still probably imperfect though. Beyond that, this is part of the intention behind the hypothetical FITSData container I've mused about before. This would be an array-like object used for FITS arrays that would include the book-keeping about things like this.

Makes sense -- so if, in a test, I want to check that data has been scaled, I could do something like ('BZERO' not in header) or (data.dtype is np.dtype(np.uint16)) ?

Of course, that would only work if you knew the data had to be rescaled in the first place. If it didn't then that test (at least when applied to floats) would result in a false-positive.

Maybe something we could do for now, until there's a better solution, is to publicly expose some of the the _ImageBaseHDU class's external bookkeeping w.r.t. scaling. Will have to think about that. I think it could help.