Check that the SIMPLE card is present - #10895

Do we really need the ignore_missing_simple option? As I see it this is just one more verification, and doesn't need a dedicated argument.

I still have an interface in mind for how I would like to better manage FITS standard verification (whether I will ever have time to try to implement it is another story, admittedly).

In other words, I would rather see this implemented within a verification framework (which currently does not exist in a consistent manner). In the meantime, instead of an OSError I think this should raise a VerifyError.

TL;DR +1 to adding this (I believe it has been requested in the past), but I think we can make it even simpler (no pun intended :)

I very much agree with @embray's overall approval and suggestions.
The main open issue seems to be whether we want a specific new keyword ignore_missing_simple for this; this is only one specific validation, but the only other one that can be controlled with its own kwarg I found is ignore_missing_end. At the very least I think the two should follow each other in the docstring. ignore_missing_end then is silently (in **kwargs) handed through to _BaseHDU._readfrom_internal, so an obvious question is whether both checks could not be done consistently in one place. But that might not be feasible as the full header has to be read in before detecting the missing 'END', and I see the value of having an immediate validity check on the first 80 bytes.

I'd say the underlying policy question is how differently we want to treat different levels of standard violations. Per the current standard (§ 4.4.1.1) a minimum requirement is actually having

SIMPLE = T
BITPIX = i
NAXIS = n
NAXIS1
...
NAXISn
...
END

and per this PR various violations of this standard would be treated as

1. No SIMPLE card: "not a FITS file"

2. SIMPLE != T: _NonstandardHDU

3. No END card: OSError

The latter two should probably be treated as the same level, and as pointed out in #10895 (comment) as VerifyError pending a consistent validation framework. I guess violations of the NAXIS standards may trigger validation errors later in the parsing.

Then it remains open if the first case should be treated as a more serious violation (so perhaps we could still stick with two kwargs, one to allow general nonstandard HDUs including SIMPLE = F, and one for the entirely missing SIMPLE. From what I gleaned from the discussion on #3941, the latter unfortunately still occur quite frequently.

Final question: how does this go along with the still existing

Re ignore_missing_simple: it's simply the same idea as ignore_missing_end, io.fits is able to read invalid files and has done so for a long time, and I'm pretty sure there will be files without the SIMPLE card. So basically it's just a way to allow users reading their broken files to fix them. And we just got an example today where io.fits is able to read a really broken file : #10905

As I see it this is just one more verification, and doesn't need a dedicated argument.

Yes maybe, but at this stage of the parsing using the verification system is not really an option. It would be great to refactor the verification system, but it's a quite lot of work, unlikely to happen soon ;).

In the meantime, instead of an OSError I think this should raise a VerifyError.

I choose OSError for consistency with OSError('Empty or corrupt FITS file'). The aim of this change is mostly to raise a better error when users load a file that is not FITS, so for me it is actually a bit different from a verification error which applies to "FITS files with problems". If we detect what seems to be a non FITS file, we should be consistent with the other OSError.

so an obvious question is whether both checks could not be done consistently in one place. But that might not be feasible as the full header has to be read in before detecting the missing 'END', and I see the value of having an immediate validity check on the first 80 bytes.

Yep, I guess you got the answer ;).

I guess violations of the NAXIS standards may trigger validation errors later in the parsing.

Yes this one is handled later by the verification system, where we also fix some common issues like using strings instead of integers (for NAXIS).

Final question: how does this go along with the still existing

The code you are pointing is only to detect FITS file for the unified IO (i.e. with Table.read), when using a file object.

@embray @dhomeier : So, I guess the main point remaining is whether to keep ignore_missing_simple or not. I'm happy to remove it if you think it's not needed. I added it because I fear the variety of invalid FITS files lying around, but I guess we can also tell people to fix their files if they miss the SIMPLE card.

@embray @dhomeier : So, I guess the main point remaining is whether to keep ignore_missing_simple or not. I'm happy to remove it if you think it's not needed. I added it because I fear the variety of invalid FITS files lying around, but I guess we can also tell people to fix their files if they miss the SIMPLE card.

Not sure how feasible that is if there are, as it seems, lots of them even coming out of major pipelines, where people have little control of the format they get. Can any of our FITS utilities fix it without requiring an expert user? Then I'd vote for that option, but otherwise I am fine with offering the kwarg in fitsopen. I think the debate was more on how many individual override options we need, but since this a fairly fundamental one and implemented very early on, it seems legit to give it its own option for now.

The non-ascii test issue should still be resolved.

I checked and the non-ASCII case is tested by test_header::test_invalid_characters.
Yes, that's the point of having this parameter. To offer the possibility to read a file missing SIMPLE if it can be read, because then reading and writing it with Astropy would allow to fix it.

Yes, that's the point of having this parameter. To offer the possibility to read a file missing SIMPLE if it can be read, because then reading and writing it with Astropy would allow to fix it.

Yes, fitscheck currently cannot fix this (it currently cannot even handle non-standard headers like SIMPLE = F or missing END card); basically to deal with missing SIMPLE would most likely require a new option for opening the file with ignore_missing_simple, so fitsopen would need that option anyway.

+1

It seems there are still issues with the treatment of invalid headers:
With SIMPLE = F in the primary header the first HDU is opened as a _NonstandardHDU but then parsing apparently stops, reading only the first HDU. And the calling code does not handle this well, e.g. fitscheck is failing on such a file with

OK '_broken.fits'
EXCEPTION '_broken.fits' .. '_NonstandardHDU' object has no attribute 'add_checksum'
1 errors

This behaviour is not affected by this PR and rather belongs in a separate issue, but probably confirms the need for a more consistent verification framework @embray pointed out.

and I'm pretty sure there will be files without the SIMPLE card

I'm not so sure. Of all the crazy, bogus, busted FITS files I've seen, I have never seen one like that. It should just not be treated as a valid FITS file.

Also, if I had the opportunity to overhaul the FITS verification framework I would also remove the ignore_missing_end keyword argument (after deprecation of course). I don't remember why it was added exactly, except that long ago somebody needed it because there were some such observatory that was producing such invalid files. But I've never heard of a missing SIMPLE keyword except for files that are not FITS files at all. I'd prefer to not add another keyword argument that isn't known to be actually needed until and unless a specific reason for it is given for a real world file. Otherwise it's just additional stuff that might be removed later.

and I'm pretty sure there will be files without the SIMPLE card

I'm not so sure. Of all the crazy, bogus, busted FITS files I've seen, I have never seen one like that. It should just not be treated as a valid FITS file.

Maybe I misinterpreted some comments in the earlier discussion like #3941 (comment) when I concluded that they were not even uncommon.
If there is consensus that there is no necessity to handle such broken files at all, of course the options could be removed/deprecated. If some files so seriously broken do exist after all, the main problem to me is that I don't see much of a middle way between

1. Offering a fairly convenient option to open them almost like a valid FITS file and

2. Telling users to ask whoever has originally created the files to provide a valid version, or write some low-level cfitsio wrapper themselves to fix them "manually".

I think some possibilities to make this support slightly less "official" could be

1. Stick to only a single option like ignore_fits_violations to make it very clear those are non-standard files.

2. Offer it only at a lower level like HDUList.fromfile or HDUList._readfrom – not sure if this is good policy?

@dhomeier You raise some good points. My thinking was that the string SIMPLE = T is the FITS magic string identifying a file as a FITS file. But nowhere does the FITS Standard make this explicit--this is only specified in RFC 4047 which is not incorporated into the FITS Standard. The Standard does say that the primary HDU must begin with SIMPLE = T in order to conform to the FITS Standard, but if it's missing that does not mean the file is not still in some sense a non-standard-compliant FITS file (there are plenty of other pseudo-FITS files out there that also don't perfectly conform to the standard, yet we still read them).

It's also a standard violation that can easily be "fixed", as we do with other violations, so long as the rest of the file "looks like" a FITS file (e.g., the first 80 bytes at least parses as a valid header card of some kind).

My other concern about adding an explicit option for this is I would rather see it handled in the context of a more general validation framework that also has the ability to fix simple errors like this as long as the rest of the file is readable. Years ago I started on that project but never completed it, and I think it would have to be resurrected from the ground up. It consisted of two layers:

• Checking the basic syntax of FITS files, were all possible syntax violations were classified explicitly, and with the ability to register custom fixers for specific violations (including many built-in fixers for violations where the fix is fairly unambiguous). This of course already exists on some level in astropy.io.fits, but it's scattered about the code in an ad-hoc manner that's hard to reason about.
• A higher-level "FITS Schema" that could be used to validate the structure of otherwise syntactically correct headers.

TL;DR, absent time to resurrect and complete that work though, I agree it would be unfair to say "no new arguments can be added to fits.open() because it might be superseded by future work that may or may not ever be completed. So I rescind my objection to adding the argument for now. It can simply be deprecated along with ignore_missing_end if the verification framework is ever overhauled.

(Rebased and moved the changelog entry to 4.3).

I would have been happy to remove this option, but it seems we now agree on keeping it, and we could indeed deprecate/remove it if one day there is a better verification system. So I think it's good to go now ?

This PR triggered a CI failure in Vaex, fix in vaexio/vaex#1490

Our header had:

b'SIMPLE  = T                   '

Our header/value was aligned to the right with spaces, I couldn't find that in the spec/rfc that it had to be that way, but in case it does not, this check might have been too strict.

SIMPLE is a mandatory keyword so it's value must be in Byte 30: (from the FITS standard, https://fits.gsfc.nasa.gov/fits_standard.html)

RFC 4047 also specifies the magic number : (https://datatracker.ietf.org/doc/html/rfc4047)

Thanks, then it was simply an error on our side!

It was actually a good question, finding the justification was not so easy ;)

Yeah, good to have this sorted out, thanks for that!

I have a similar issue where the first 30 bytes of some external data are

b'SIMPLE  = T /Tape is in Fits f'

instead of the expected

b'SIMPLE  =                    T'

so I got OSError: No SIMPLE card found, this file does not appear to be a valid FITS file, which had me scratching my head for a while, as I was sure the file had a SIMPLE card and Astropy used to recognise these files in previous versions. In fact, if we use the new ignore_missing_simple option, Astropy 4.3 will still eventually figure out that the file does have a SIMPLE card (due to the techniques it uses to interpret non-standard files).

Don't get me wrong I completely agree that the file I have is non-standard, but it seems to me that this check is overly strict if our goal is just to be sure that we're dealing with a FITS file. Keeping in mind this philosophy:

I believe we could make a much better attempt at finding a SIMPLE card before giving up. Instead of:

I like that, a warning would be nice though so that people are aware the fits image is not created according to the standard.

@saimn @embray Could you please give your opinion on my comment above? This breaks backwards compatibility in my use case,
Would you prefer that I create a separate issue? Thank you.

Sorry for the delay, last 2 weeks have been quite busy.
I think I would prefer to keep the check as is, at least for now. As described above this card is well defined and is the only way to identify a FITS file. io.fits is more flexible for the other cards, and tries to fix them if possible, but the case of SIMPLE is a bit different given its role to identify a FITS file.
Depending on the number of complaint we get we could revisit and issue a warning if SIMPLE is there but not valid, but I think for now ignore_missing_simple is a good enough workaround (and let's hope this encourages people to fix their broken files or report to the producer!).

No worries, take your time. Correct me if I'm wrong, but from what I can tell the main goals here were that, when io.fits faces a file that it cannot interpret as FITS, it should 1) stop the attempt earlier and 2) raise a better/clearer error. My problem with the current change is that it triggers the error on some non-standard files that io.fits can interpret as FITS. The change that I'm suggesting would still achieve those two main goals, while being a bit more lax with the verification. I think this would be more in line with the stated philosophy of being "tolerant with input and strict with output" and not giving up as soon as the file is deemed non-standard. I believe issuing a warning would make it more similar to the behaviour of other standard violations, e.g. null bytes being used for padding. To be clear, the change I proposed would only affect cases where SIMPLE is present but the spacing is non-standard, and it would still raise an error if SIMPLE is non-existent.

Maybe I understood it incorrectly and the goal here is specifically to discourage the usage of non-standard SIMPLE cards? I can see the benefits there, but I would guess that most people that have to deal with non-standard files aren't the ones actually producing those files... and if they are, a warning can still get the message across.

Either way, even if this behaviour isn't changed, could we at least include a suggestion to use ignore_missing_simple in the error message? This could help reduce the confusion of why a file suddenly cannot be read after an update. This was especially confusing in my case where the SIMPLE keywords was there, just in a non-standard way. Or at the very least the error message could be different in that case.

This is a significant break in backwards compatibility for our code that processes archival data that cannot be easily changed. We will have to update hundreds of lines of code -- every fits.open we have in 11 different packages -- in order to set ignore_missing_simple=True. Would it at least be possible to make True the default value to preserve handling for files that can be read as FITS?

It is not impossible to add a config item to control this, though it does add to code complexity... @melanieclarke , can you please open a new issue and link to this one? Thanks.