Question: Why is io.fits convenience 'getdata' much faster than fits.open()?

>>> import sys
>>> print(sys.version)
2.7.13 |Continuum Analytics, Inc.| (default, Dec 20 2016, 23:05:08) 
[GCC 4.2.1 Compatible Apple LLVM 6.0 (clang-600.0.57)]
>>> import astropy 
>>> astropy.__version__
u'2.0.dev17848'
>>> import timeit
>>> timeit.timeit("h=fits.open('jcdua3f4q_flc.fits'); d=h[3].data; h.close()",
... setup="from astropy.io import fits", number=1000) 
22.351375102996826
>>> timeit.timeit("d=fits.getdata('jcdua3f4q_flc.fits', ext=3)",
... setup="from astropy.io import fits", number=1000)
8.482270956039429

How is this possible and what can be done to make "regular" fits.io operations as fast as "convenience" functions?

That's really weird because essentially _getext (what is called by getdata) just does fitsopen and then extracts the relevant data. So these should be equivalent.

Is caching done and affected the second test? Try do the timing in different Python sessions?

Well, timing IO is tricky at best but I have similar timings.

@pllim

@pllim Order of operations as well as number of sessions does not affect the outcome

@MSeifert04

@MSeifert04 wrote:

That's really weird because essentially _getext (what is called by getdata) just does fitsopen and then extracts the relevant data. So these should be equivalent.

A possible hint may come from:

>>> timeit.timeit("h=fits.open('jcdua3f4q_flc.fits', memmap=False);d=h[3].data",
... setup="from astropy.io import fits", number=1000) 
11.85682988166809

So, it appears that close() is responsible for most of the differences (but not all).

not closing the file halves the timings?

Yes. However, getdata() does close the files so things are still strange. However, maybe this last test could point to a culprit?

Profiling it would give you some answers.

@embray

timeit.timeit("h=fits.open('jcdua3f4q_flc.fits', memmap=False); d=h[3].data; h._close()",
 setup="from astropy.io import fits", number=1000)
12.638292074203491

It seems that close() is much slower than _close(). The reason seems to be due to the following code in close():

       if closed:
            # Get the actual current number of HDUs that have been loaded
            total_read = list.__len__(self)

            # Now read in any remaining HDUs
            while self._read_next_hdu():
                pass

It is reading all HDUs regardless of what the user needed! Why??? @embray

This reading of all HDUs defeats any sort of "lazy" reading in fits.

The explanation is in a comment in getheader:
https://github.com/astropy/astropy/blob/master/astropy/io/fits/convenience.py#L115

    # Use _close instead of close to close without loading any
    # remaining HDUs for pre-lazy-loading backwards compatibility
    # In other words, when the full HDUList is opened by a user they
    # previously expected to be able to look at arbitrary HDUs even
    # after the file was closed, but for getheader and other convenience
    # functions this is irrelevant

Are we then supposed to use "private" _close() instead of close() if we no longer need HDUList?

@jhunkeler

It seems that close() was re-designed in 5c66c2d and this has destroyed any advantage that memmap=True might have had (which kind of was supposed to be a sort of "lazy" loading).

The irony of this comment #4634 (comment) can't be avoided.

CC: @jhunkeler

@mcara

@mcara I think you pinged the wrong one, juliantaylor submitted the PR.

I thought we added a deprecation warning when accessing closed files. However I can do that without any problems (and/or warnings).

@MSeifert04

@MSeifert04 No, I was having a conversation with @jhunkeler a few days ago about related issues.

@MSeifert04

@MSeifert04 - The commit mentioned by @mcara added the warning: 5c66c2d#diff-817bd3a22004516f0dd0080e2cd0d9baR1028

Also, as written in this commit, this is only for headers. Reading data after close was already disabled, so I don't think there is an issue with memap, @mcara ?

And instead of using directly _close, an option would be to add a keyword to disable this behavior:

hdulist.close(no_really_do_not_load_all_other_hdus_please=True)

(This is also suggested in the commit message)

@mwcraig

Eh, here is the issue mentioned previously:
#5582

However in #5589 @mwcraig decided not to fix close().

Due to this weird implementation of "lazy", now regular functions are slower than "convenience" functions.

If it can be removed for v2.0 it's even better indeed.

Should we close this as duplicate of #5582 then?

@mwcraig

However in #5589 @mwcraig decided not to fix close()

IIRC, that came down to running out of time before feature freeze. 😀

In addition, the goal was not to change the previous behavior (in which headers remained available even after closing).

hdulist.close(no_really_do_not_load_all_other_hdus_please=True)

I'd rather just remove this old behavior in 2.0.

Should we close this as duplicate of #5582 then?

Sure; could you please also add a note over there referring to this issue? Would do it myself but about to run to a meeting...

Due to this weird implementation of "lazy", now regular functions are slower than "convenience" functions.

In fairness, they're just as slow as they always were ;)

Navigation Menu

Repository navigation

Question: Why is io.fits convenience 'getdata' much faster than fits.open()? #5918

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Question: Why is io.fits convenience 'getdata' much faster than fits.open()? #5918

Description

Activity

MSeifert04 commented on Mar 28, 2017

pllim commented on Mar 28, 2017

MSeifert04 commented on Mar 28, 2017

mcara commented on Mar 28, 2017

mcara commented on Mar 28, 2017

MSeifert04 commented on Mar 28, 2017

mcara commented on Mar 28, 2017

pllim commented on Mar 28, 2017

mcara commented on Mar 28, 2017

mcara commented on Mar 28, 2017

saimn commented on Mar 28, 2017

mcara commented on Mar 28, 2017

mcara commented on Mar 28, 2017

MSeifert04 commented on Mar 28, 2017

MSeifert04 commented on Mar 28, 2017

mcara commented on Mar 28, 2017

saimn commented on Mar 28, 2017

mcara commented on Mar 28, 2017

saimn commented on Mar 28, 2017

pllim commented on Mar 28, 2017

mwcraig commented on Mar 28, 2017

embray commented on Mar 30, 2017

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions