Few optimizations for FITS tables by saimn · Pull Request #4985 · astropy/astropy

saimn · 2016-05-27T10:16:59Z

I have FITS files with a lot of extensions, lot of tables with many columns (a few thousands sometimes ...) and it takes a significant time to load everything, so I tried to find some optimizations (btw snakeviz is very nice to look at profiling results !).
The changes here could be seen a micro-optimizations, but they concern methods which are called many times, so it makes a difference in the end.

Reference: Astropy 1.2.dev15480, Time to load my file : 2.8s
Change on _is_int (ncalls=1621317). Time after : 2.64 s
Change on Coldefs.__getattr__ (ncalls=2673). Time after : 1.73s
Change on ColumnAttribute.__get__ (ncalls=1626425). Time after : 1.6s

Maybe @embray if you have some time ?

bsipocz · 2016-05-27T13:22:57Z

 # lists of column/field definition common names and keyword names, make
 # sure to preserve the one-to-one correspondence when updating the list(s).
 # Use lists, instead of dictionaries so the names can be displayed in a
 # preferred order.


nitpicking: change list to tuples in the comment, too.

pllim · 2016-06-01T13:46:39Z

@@ -42,6 +42,8 @@ class StringIO(object):
 elif six.PY2:


Not related to this PR, but I just notice that this logic here is not compatible with Python 4.

Removed the PY3 test.

saimn · 2016-06-02T10:33:57Z

@pllim @MSeifert04 - With 3af359c I now cache the lists of attributes in ColDefs instead of calling too many times __getattr__ (for name, format, ...). This results in a 2.5x speedup (just for this commit) ! I hope I handle correctly all the cases when the cache must be updated, at least the tests pass.

@embray - Maybe, if you want have a look ?

Master:

~/dev/astropy master*
❯ python -m timeit -n 5 -r 5 -s "from astropy.table import Table" "Table.read('table.fits')"
5 loops, best of 5: 2.44 sec per loop

This branch:

~/dev/astropy fits-table-optims*
❯ python -m timeit -n 5 -r 5 -s "from astropy.table import Table" "Table.read('table.fits')"
5 loops, best of 5: 485 msec per loop

pllim · 2016-06-02T13:54:51Z

+        """Get or update local lists of column attributes"""
+        attrs = [name] if name is not None else KEYWORD_ATTRIBUTES
+        for name in attrs:
+            setattr(self, name + 's', self.__getattr__(name+'s'))


I'm surprised PEP8 test didn't complain about lack of white space around +. Since it didn't complain, I guess we can leave this as is.

It's not switched on for E225

Fixed, though there is no real obligation for spaces around operators ;-)

If operators with different priorities are used, consider adding whitespace around the operators with the lowest priority(ies). Use your own judgment; however, never use more than one space, ...

http://pep8.org/#other-recommendations

pllim · 2016-06-02T14:05:17Z

Overall, the speed-up looks nice. Like you said, the tests passed. So unless someone takes this for a spin on a variety of FITS tables, it is good enough?

saimn · 2016-06-07T07:31:31Z

Rebased to fix conflict with master.

astrofrog · 2016-08-10T13:18:09Z

-                    attr.append(val)
-                else:
-                    attr.append('')
+                attr.append(val if val is not None else '')


@saimn - I think (val or '') would work too?

@astrofrog - I think so, but the original code uses is not None. I'm not sure if there are case where val could be a falsy value that should not be translated to a empty string. So, in doubt, better stick with the original implementation.

Yep, what @saimn said.

This is a micro-optimization but _is_int can be called many many times when loading FITS tables, so it can start to be meaningful.

embray · 2016-09-06T10:00:27Z

+        if attr == 'name':
+            del self.names
+        elif attr == 'format':
+            del self.formats


Doesn't quite work--these also need to be invalidated when columns are added/removed. Otherwise I'm fine with the general concept. My preference would still be to not make any changes in the ColDefs class and instead update the FITS_rec class to be less reliant on the ColDefs object in general (an object I would like to get rid of, ultimately).

That said, in the interest of not wasting any more of your time on this and getting it finished, if you can just fix the column add/remove bug and ensure all the tests are passing this is definitely good enough for now! 👍

I also noticed you cleaned up a little bit of additional code rot--thanks for that.

Thanks Erik ! The column addition/removal is now fixed.

When you say FITS_rec class to be less reliant on the ColDefs, I wonder for example why ``FITS_rec.namestries first to use_coldefs.names` and then `dtype.names`, and not the opposite ?

Based on our discussion here, I tried to make a few changes in fitsrec.py. This is on this gist, should I include this commit ?

replace self._coldefs.names with self.names

move indexing to self._coldefs when possible

and change the way to get names using dtype.names first

When you say FITS_rec class to be less reliant on the ColDefs, I wonder for example why `FITS_rec.names tries first to use _coldefs.names and then dtype.names, and not the opposite ?

Exactly--I think the opposite makes more sense. The way it works currently may be a remnant of when the dtype fields and the "logical" columns represented by the ColDefs class were not guaranteed to be the same. The reason for this is the unfortunate existence in FITS of "zero width" columns which must be supported--it's rare but does occur in the wild. It used to be that the Numpy dtype could not accommodate that, so there were a lot of hacks to make all the columns (including zero-width ones) represented to the user, while only representing the non-zero columns in the actual dtype.

I've gotten a bunch of patches into Numpy, however, to allow fields with zero size to work in a consistent manner, so most of those hacks have been removed (there is still one remnant of this issue: numpy/numpy#6430 Though that reminds me, the fix for that will be in Numpy 1.12 so in principle it should be possible to avoid use of the realign_dtype hack for Numpy 1.12 and up).

The patch in the gist looks good to me. I haven't tested it, and can't say with certainty that there are some dark corners lurking where that would actually break. But I don't think there--it's definitely what I had in mind.

Ok great ! I pushed the commit for fitsrec.

MSeifert04 · 2016-09-06T14:20:11Z

@saimn Since this is quite a small PR could you squash the commits into one and include a changelog entry (just to mention the improved runtime)? Maybe wait until the tests have finished to see if anything went wrong.

I'm not sure this is strictly necessary here so maybe wait if someone else wants to second that.

I'm really interested about the fitsrec-changes. I've experimented with them before and always got several failures, so I'm really interested to see if it works out. 👍

saimn · 2016-09-06T14:27:30Z

@MSeifert04 - I can probably squash 2 or 3 commits for things that are modified in another commit, but there are a number of commits I would like to keep as they address specific things and contain some information on the change.

As a descriptor it is often called (for each attribute of the column). So instead of adding each time '_', add it once.

Getting names from dtype.names should be safe and avoids accessing _coldefs. Also use directly self.names, and change several _coldefs indexing to get the column before accessing an attribute.

saimn · 2016-09-06T20:27:19Z

Ok, squashed some commits, and added changelog.

saimn · 2016-09-07T06:10:41Z

The build failure seems unrelated (something with a samp hub process: https://travis-ci.org/astropy/astropy/jobs/157994577#L2451).

embray · 2016-09-07T09:26:55Z


+- ``astropy.io.fits``
+
+  - Optimized the loading of tables with many columns. [#4985]


I think "optimized" is a strong word. It's hardly optimal. You can just say "Performance improvements"

embray · 2016-09-07T10:02:55Z

Great--thanks for putting up with my nitpicks. I think this is good!

pllim · 2016-09-07T13:55:30Z

Got approval, so merging. Thanks, @saimn !

saimn · 2016-09-07T15:46:27Z

Thanks for merging :)

taldcroft · 2016-10-14T18:03:33Z

@saimn - I'm not sure if it was exactly this PR, but reading short wide tables is way faster for me now, as in a factor of 8! (Comparing 1.2.1 to current master 1.3.dev16204). This is awesome and therefore closes #4609.

taldcroft · 2016-10-14T18:05:10Z

Specifically, for this table I get the factor of 8 improvement:

import numpy as np
from astropy.table import Table

ncols = 800
nrows = 10
cols = [np.random.uniform(size=nrows) for _ in xrange(ncols)]
dat = Table(cols)

dat.write('test_speed.fits', overwrite=True)

saimn · 2016-10-14T20:27:32Z

@taldcroft - This is typically the use case I was addressing here, tables with a many columns, so I'm glad it is useful to someone else :)

embray · 2016-10-17T12:05:34Z

Thanks @taldcroft for the testimonial--I bet it was this PR!

bsipocz reviewed May 27, 2016
View reviewed changes

pllim added the io.fits label Jun 1, 2016

pllim reviewed Jun 1, 2016
View reviewed changes

pllim added the Performance label Jun 1, 2016

pllim reviewed Jun 2, 2016
View reviewed changes

astrofrog added this to the v1.3.0 milestone Jun 2, 2016

saimn force-pushed the fits-table-optims branch from 3bdecf5 to 8fc380a Compare June 7, 2016 07:29

astrofrog reviewed Aug 10, 2016
View reviewed changes

Avoid addition at each call of _is_int

6f0850c

This is a micro-optimization but _is_int can be called many many times when loading FITS tables, so it can start to be meaningful.

saimn force-pushed the fits-table-optims branch from 8fc380a to a3e8f7c Compare September 5, 2016 15:16

embray reviewed Sep 6, 2016
View reviewed changes

saimn added 5 commits September 6, 2016 22:15

Minor changes

b6ccbf7

Speedup ColumnAttribute

f1f274b

As a descriptor it is often called (for each attribute of the column). So instead of adding each time '_', add it once.

Remove unused variables

7a57887

Cache lists of names and formats at the Coldefs level

de5b716

Change FITS_rec.names to use names from recarray by default

9388498

Getting names from dtype.names should be safe and avoids accessing _coldefs. Also use directly self.names, and change several _coldefs indexing to get the column before accessing an attribute.

saimn force-pushed the fits-table-optims branch from 52bd47b to a7ae89e Compare September 6, 2016 20:25

MSeifert04 mentioned this pull request Sep 6, 2016

Make code Py4-compatible #5307

Closed

embray reviewed Sep 7, 2016
View reviewed changes

saimn force-pushed the fits-table-optims branch from a7ae89e to cf0b815 Compare September 7, 2016 09:47

Add changelog [skip ci]

0a58c66

saimn force-pushed the fits-table-optims branch from cf0b815 to 0a58c66 Compare September 7, 2016 09:48

pllim added the Affects-release label Sep 7, 2016

pllim merged commit c18eb28 into astropy:master Sep 7, 2016

saimn deleted the fits-table-optims branch September 9, 2016 13:31

taldcroft mentioned this pull request Oct 14, 2016

Performance regression when reading a short and wide FITS binary table #4609

Closed

saimn mentioned this pull request Nov 18, 2021

Why looping through hdus at pos=2 in fits files is much slower than looping through hdus at pos=1? #12491

Open


		- ``astropy.io.fits``

		- Optimized the loading of tables with many columns. [#4985]

Conversation

saimn commented May 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saimn commented Jun 2, 2016

Uh oh!

pllim Jun 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim commented Jun 2, 2016

Uh oh!

saimn commented Jun 7, 2016

Uh oh!

astrofrog Aug 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MSeifert04 commented Sep 6, 2016

Uh oh!

saimn commented Sep 6, 2016

Uh oh!

saimn commented Sep 6, 2016

Uh oh!

saimn commented Sep 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

embray commented Sep 7, 2016

Uh oh!

pllim commented Sep 7, 2016

Uh oh!

saimn commented Sep 7, 2016

Uh oh!

taldcroft commented Oct 14, 2016

Uh oh!

taldcroft commented Oct 14, 2016

Uh oh!

saimn commented Oct 14, 2016

Uh oh!

embray commented Oct 17, 2016

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

pllim Jun 2, 2016 •

edited

Loading

astrofrog Aug 10, 2016 •

edited

Loading