Repository navigation
Why looping through hdus at pos=2 in fits files is much slower than looping through hdus at pos=1? #12491
Description
Description
I am looping through the hdus of astro fits files I am analyzing. When I loop through the hdus at position 1 (hdu=1) with astropy.table.read the time for that is reasonably short. But if I do the same for the hdus at position 2 (hdu=2) the time is 10 times slower, despite that the size of hdu=2 is smaller than for hdu=1. The tables/hdus at position 2 are smaller (~27 KB) than the tables at position 1 (~130 kB). Further, tables at position 1 have shape 2833 rows * 8 columns and tables at position 2 have shape 1 row * 126 columns. The time matters to me, because I need to loop through tens of thousands of files eventually, so in the magnitude of hours.
Example file can be this one: https://data.sdss.org/sas/dr16/eboss/spectro/redux/v5_13_0/spectra/lite/3699/spec-3699-55517-0420.fits
Expected behavior
Looping should be the same or maybe faster for hdu=2.
Actual behavior
Looping through hdu=2 is much slower (~10 times).
Steps to Reproduce
To reproduce you can do
from astropy.table import Table
for file_name in fits_files_list:
table1 = Table.read(file_name, hdu=1)
# fits_files_list is a list of containing the file paths to fits files. If you want to reproduce my code you can download an example from here:
# https://data.sdss.org/sas/dr16/eboss/spectro/redux/v5_13_0/spectra/lite/3699/spec-3699-55517-0420.fits
# this is the sort of files I am working with now, they all have the same structure
Change then hdu=1 to hdu=2, keep track of the execution time and note that the second time it is ~10 times slower.
System Details
The details to my system/environment are:
Linux-5.11.0-40-generic-x86_64-with-glibc2.10
Python 3.8.5 (default, Sep 4 2020, 07:30:14)
[GCC 7.3.0]
Numpy 1.19.1
astropy 3.2.3
I have to erfa or scipy installed.
Activity
github-actions commentedon Nov 18, 2021
Welcome to Astropy 👋 and thank you for your first issue!
A project member will respond to you as soon as possible; in the meantime, please double-check the guidelines for submitting issues and make sure you've provided the requested details.
GitHub issues in the Astropy repository are used to track bug reports and feature requests; If your issue poses a question about how to use Astropy, please instead raise your question in the Astropy Discourse user forum and close this issue.
If you feel that this issue has not been responded to in a timely manner, please leave a comment mentioning our software support engineer @embray, or send a message directly to the development mailing list. If the issue is urgent or sensitive in nature (e.g., a security vulnerability) please send an e-mail directly to the private e-mail [email protected].
pllim commentedon Nov 18, 2021
Did you try to profile the code to see where the bottleneck is for your use case? Does it help if you pass in
memmap=True?As you can see in https://github.com/astropy/astropy/blob/main/astropy/io/fits/connect.py , there are a bunch of stuff going on, so profiling your code would be the best way to figure out why.
embray commentedon Nov 18, 2021
I looked at the example file in question, and although the data in hdu=1 is ;arger, that table contains only 8 columns, whereas hdu=2 has 236 columns which is quite a lot. The time of reading the table seems to scale linearly in this case with the number of columns. In both cases they are very small tables, so the time of loading these tables is actually dominated by parsing the column data from the header, compared to actually reading the data.
We could try profiling the code and see if there any pain points slowing this down, but regardless this is an issue specific to these files, and the large number of columns in those tables.
embray commentedon Nov 18, 2021
From a cursory look there are a couple obvious problems. One is that when loading data from a binary table it actually goes through
ColDefs.__init__twice, redundantly. This could and should be fixed. The other issue is, I as suspected, that just initializing theColDefsat all is slow, in both cases. A large portion of that time is dominated by either reading header keywords (not a problem usually unless there are hundreds of columns, and thus N*hundreds of header keywords to read), and constructing theColumnobjects themselves. I think in both cases there are opportunities for optimization, but it will require a closer look.saimn commentedon Nov 18, 2021
Indeed the number of columns must be the reason reading this HDU is slower. I'm not sure there are easy optimizations there (since I remember struggling with this a long time ago, #4985 :)). One alternative way however would be to use the
io.fitsAPI instead ofTable, since there is probably a non-negligible cost in theBinTableHDUtoTableconversion.Nestak2 commentedon Nov 18, 2021
Thanks for all the answers! @embray @saimn I actually don't need data from each of the 126 columns in hdu=2, I only need ~10 columns from that table. Is there some hack in that connection that I could use?
embray commentedon Nov 19, 2021
@saimn What I found in investigating this is that using
io.fitsis no faster. Converting to Table incurs some overhead but not as much.What I found was that in
_TableLikeHDU._get_data, aColDefsfor the table gets constructed twice. The first time is when it callsself.columnswhere it constructs theColDefsfrom the header. It then uses this to determine the appropriate dtype, as well as some other processing of the data format.Then on this line it views the array as a
FITS_rec(a class that is pretty vestigial at this point, but will take some effort to get rid of). This results in constructing a newColDefsobject, but this time from the dtype instead of the header.This redundancy has significant overhead for a case like this, and should be done away with, though I'm not sure how yet.
Both of the methods used here for constructing
ColDefs(from the header, and then from the dtype) have opportunities for improvement as well.embray commentedon Nov 19, 2021
@Nestak2 In order to know where the columns are in a table the column information has to be parsed anyways. The only way you could work around that is if you know each table has the exact same layout, then I guess you could do something.
Though that question raises for me one possibility of optimizing cases like this.
If you are looping over a large number of similar files (which is not such an uncommon case), and all the files contain tables with the exact same structure, we could have a system for caching table column structures, e.g. based on the table keywords from the header.
Do you know for sure in your case that all/most of the tables have the exact same columns?
embray commentedon Nov 19, 2021
Another interesting thing I found while examining the performance here is that lookup of header keyword values is actually being largely dominated by, of all things, the astropy config system. There are various config settings that are checked when parsing headers, some of which will get checked for every single keyword.
On some level this is necessary since astropy configs are supposed to be reloadable at runtime. But I wonder if for parsing headers it would make sense to allow config changes between reading individual values from the same header.
pllim commentedon Nov 19, 2021
Where exactly in the code is config heavily used --
tableorio.fits?@bsipocz , @saimn , and I were just discussing on how we shouldn't use it so much the other day. We should only use it for something that is "set and forget" for a session. It is not a replacement for
**kwargs.embray commentedon Nov 19, 2021
You can manually reset a specific config variable, but there's nothing "magic" where it changes automatically if you edit the config file at runtime. You still have to call
conf.reset('option')pllim commentedon Nov 19, 2021
I am not saying there is magic. I am wondering if we can stop using config for better performance, but this is FITS, so maybe not...
bsipocz commentedon Nov 19, 2021
That's a good test and reminder that we should then indeed use the config system to set the defaults in init, but not to directly set the default values for method kwargs. (Though I don't think it should be an issue for astroquery where we end up querying a remote server, in no way should any of those methods be limited by the config system)
Nestak2 commentedon Nov 22, 2021
@embray Thanks, I think that most if not all tables have the exact same columns, but I am not 100% sure, because I am just starting to use them as a PhD student. More precisely, the files I am working with are fits-files from the SDSS archive, all of them containing spectral info, limited to only QSO objects and part of the eBOSS survey. Below I have the top 5 URLs from ~20000 or even more, that I would need to work with, if you want to check them yourself.
If you want to check them in another way - I query the files to work with with the SQL query tool of Cas jobs using this SQL code:
Any ideas how to optimize the time for looping through tables with exact same columns? Tnx