Repository navigation
Bad checksum not being overwritten on call to "writeto" #5902
Description
Activity
Trying my luck on additional @embray and @MSeifert04 .
cadairis not involved in FITS and I have little to say about this topic.What would be the correct behaviour here? I mean
output_verifyisFalseand thechecksumparameter was "sort of"True. However thechecksumparameter only says "When True adds both DATASUM and CHECKSUM cards to the headers of all HDU’s written to the file.", does that mean it should always overwrite the existing DATASUM and CHECKSUM or just add them if not present?I mean the test case is a bit theoretical: If you explicitly garbage the CHECKSUM and don't check it when reading the file what's the point about "correcting" it when writing the file again?
The documentation of
checksuminfits.openis this:If True, verifies that both DATASUM and CHECKSUM card values (when present in the HDU header) match the header and data of all HDU’s in the file. Updates to a file that already has a checksum will preserve and update the existing checksums unless this argument is given a value of ‘remove’, in which case the CHECKSUM and DATASUM values are not checked, and are removed when saving changes to the file.
That actually means (in my opinion) that when you suspect a "garbage" checksum you should provide
checksum='remove'when reading the file andchecksum=Truewhen writing it again.I'd have to take a closer look, but it's a known (to me) issue that checksum handling is confusing. I believe there may already be some open tickets about this either here or in the old PyFITS repo. Which is not to say the behavior isn't deliberate. It's just confusing.
does that mean it should always overwrite the existing DATASUM and CHECKSUM or just add them if not present
I think this means it should overwrite. DATASUM and CHECKSUM are two of those "special" header keywords that nobody should ever touch manually without very good reason to.
- My other issues and pending PRs are all related to this issue. Pending because of a number of confusions. I'll go back to them and post a compiled comment.…On May 3, 2017 6:26 PM, "Erik Bray" ***@***.***> wrote: I'd have to take a closer look, but it's a known (to me) issue that checksum handling is confusing. I believe there may already be some open tickets about this either here or in the old PyFITS repo. Which is not to say the behavior isn't deliberate. It's just *confusing*. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#5902 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AOc7f1Ua-5w_toR1fhUdja8fzypmSnjjks5r2Hl6gaJpZM4Mm4Hw> .
Astropy v2.0
Test files used:
io/fits/tests/data/test0.fits- ImageHDU extensionsio/fits/tests/data/checksum.fits- A BinTableHDU extensionWhile working on #5874 I came across this :-
CHECKSUMheader ofchecksum.fitsand addedCHECKSUMtotest0.fitsand modified it'sCHECKSUMheader too. (i.e. 16 letter long garbage value).CHECKSUMs were written only fortest0.fitsThis corrected the Garbage checksums.
and this didn't.
The above one works if something like -
>>> hlist = fits.open('./tests/data/checksum.fits', checksum=True)is used.Is this because of different types of extensions that are present?
On digging a bit this is happening because
hdu._header._modifiedisFalseforchecksum.fitscase.https://github.com/astropy/astropy/blob/master/astropy/io/fits/hdu/base.py#L522