Allow HDUs to not be loaded at close() to gain full benefit of lazy loading

#5065 implemented lazy loading of HDUs in fits files. But it did it in a way that maintains backwards compatibility at the expense of performance (See #5065 (comment) and other discussion in #4634). So we should consider removing that compatibility bit. It's been deprecated for 1.3 so it might make sense to slate this for 2.0...?

Also brought up in #5918.

Several comments stemming from the discussion in #5918.

It seems that just inquiring the length of the HDUList (see https://github.com/astropy/astropy/blob/master/astropy/io/fits/hdu/hdulist.py#L908) results in loading all headers. This means that this code https://github.com/astropy/astropy/blob/master/astropy/io/fits/hdu/hdulist.py#L911 may not be needed.
If backward compatibility is important, why not be lazy to the very end: instead of creating Headers before closing the file, why not simply store the headers in a raw format (strings, bytes, etc.) and create Header only when requested (i.e., using HDUList.fromstring()).

It seems that just inquiring the length of the HDUList (see https://github.com/astropy/astropy/blob/master/astropy/io/fits/hdu/hdulist.py#L908) results in loading all headers.

I mean, currently that sort of has to be the case. You can't know the length of a FITS file (in terms of number of HDUs) without scanning through the entire FITS file and determining where the headers are (which you can't even really do without ensuring that they are at least somewhat valid headers). This is one of the long-time shortcomings of FITS. I once started working on a proposal for a FITS extension to include a tabulation of HDUs in the primary header, but it never got off the ground. Maybe you should!

You should also have a look at #5593, which nobody's really commented on, but there I made a number of suggestions for performance improvements, in particular with regards to headers, so as to speed up scanning through FITS files to find all the HDUs without doing as much work on header parsing.

If backward compatibility is important, why not be lazy to the very end: instead of creating Headers before closing the file, why not simply store the headers in a raw format (strings, bytes, etc.) and create Header only when requested (i.e., using HDUList.fromstring()).

That's actually mostly what it does, and has done for a long time. Almost all the header card are not fully parsed at first. See again #5593 where I explain this.

As to "backwards compatibility" the only thing it's maintaining backwards compatibility for is a mispattern (as I demonstrated) like:

>>> hdulist = fits.open(filename)
>>> # do some stuff
>>> hdulist.close()
>>> print(hdulist[3].header)

Originally, of course, I didn't want to do this because it hurt the lazy-loading, but in practice I found (just by searching the internet!) that there was a lot of code out there this would impact. So please do consider that I did my research on this and was doing my best to balance performance enhancement, while still supporting the legacy of a decade worth of code out there based on a PyFITS design that, I think, was not well considered early on.

One thing that could be added for the next Astropy feature release, without even removing the deprecation warning/backwards compat, might be to add yet another flag to pyfits.open()--I'm not sure yet what to call it--that was enforce the new semantics of "when I do hdulist.close() I really mean close the file and don't read any more bytes out of it. If you enable that explicitly then it isn't breaking backwards-compatibility and you will retain all the (current) benefits of lazy-loading. A global variable could be added for that to make it the default behavior too. I would strongly suggest that.

(I should add, I had to argue a bit with a few people that the additional complexity of this implementation was worth it, because fits.open() shouldn't be any slower than fits.getdata() or fits.getheader()--the fact that you're complaining that this isn't the case is somewhat vindicating for me 😬 )

One more comment--in principle you could add the flag I mentioned to the v1.3.2 release. Yes, it's a new feature but it wouldn't break anything, and might just be used, at least for now, as a performance enhancement for those who need it.

@embray

You should also have a look at #5593, which nobody's really commented on, but there I made a number of suggestions for performance improvements

@embray : I had a look recently, and it seems really interesting. So I hope to find some time to give it a try.

@saimn

@saimn Thanks! I really wish I had more time to work on it myself.

@embray

@embray : Many thanks for your comments, info!

Can we state on what should be done for 2.0 ?

Option 1: Keep the current behavior (force the reading of all HDUs on close) for 2.0, if we fear that too much code depend on this. In this case, I'm willing to add the option to explicitly disable this behavior.
Option 2: Remove this compatibility behavior. In this case users can use lazy_load_hdus=False which is equivalent to the old behavior by forcing the reading of all HDUs.

Option 1 seems to be a safer option but please "...add the option to explicitly disable this behavior."

In the age of big data, option 2 makes more sense to me.

@MSeifert04

@MSeifert04 ? @jhunkeler ?

Do we expect this to be an issue for much code? Can we estimate it somehow?

No2 sounds better to me, too and as v2.0 will be a new LTS release, making some API and behaviour changes is acceptable.

@bsipocz

@bsipocz In light of the 2.0 target, I agree Option 2 makes sense. It would be good to add a global option for changing the default behavior (e.g. set lazy_load_hdus=False by default globally). But other than that, v2.0 seems like a good opportunity to break existing code like this.

It's difficult to estimate exactly how much code would be broken--most of it would be sloppily written one-off scripts that have never seen the light of day, but that people are nevertheless using for their analysis.

Remove compatibility code which forced loading all HDUs on close

Navigation Menu

Repository navigation

Allow HDUs to not be loaded at close() to gain full benefit of lazy loading #5582

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Allow HDUs to not be loaded at close() to gain full benefit of lazy loading #5582

Description

Activity

pllim commented on Mar 28, 2017

mcara commented on Mar 28, 2017

embray commented on Mar 30, 2017

embray commented on Mar 30, 2017

embray commented on Mar 30, 2017

saimn commented on Mar 30, 2017

embray commented on Mar 30, 2017

mcara commented on Mar 30, 2017

saimn commented on Apr 25, 2017

mcara commented on Apr 25, 2017

pllim commented on Apr 25, 2017

pllim commented on Apr 25, 2017

bsipocz commented on Apr 25, 2017

embray commented on May 5, 2017

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions