Make it possible to use Table.read on FITS files with no copying by astrofrog · Pull Request #6821 · astropy/astropy

astrofrog · 2017-11-08T19:23:51Z

Motivation

The intent of this PR was to address (at least from the FITS point of view) the issue described in #6491 - namely that when using Table.read with a FITS file, the data is currently copied, which can lead to poor performance.

Bottom line

With this PR, I can read a 1.8Gb FITS table with some string columns using:

from astropy.table import Table
t = Table.read('../Planes/fun-with-adsb/summary_full.fits')

in 2 seconds with 40Mb memory usage:

instead of 50+ seconds and 5Gb of memory:

In fact, the 2 seconds/40Mb are dominated almost entirely by the import statement, and the read statement takes 40ms. This is a speedup of almost ~1000x! Of course data will then be read as needed from disk, but this means that for accessing specific columns/rows/cells from a table, users could see significant speedup.

FITS changes

This turned out to be more complicated than I thought. The issue is the following - we can simply add copy=False inside astropy/io/fits/connect.py to deal with preventing a copy for non-string columns. However, astropy.io.fits is inefficient when it comes to string columns in that it automatically converts A columns (which are fundamentally ASCII as far as I understand) to Numpy U(nicode) types. This was described by @taldcroft in: #6122 - this has two implications: memory mapping is not respected and the whole string columns are read in, and the memory usage is 4x what it needs to be because of the conversion to unicode.

So this PR adds the ability to control whether string columns are converted to unicode in astropy.io.fits with the return_bytes option to fits.open (which should be useful in itself), and then makes it so that we use that option for Table.read.

Table changes

I also had to make it possible for add_column to take a copy argument - @taldcroft: was there a reason for not having that originally?

Time changes

Finally I noticed that there was an issue in the Time parsing from FITS files - passing a Numpy array with S type to Time doesn't actually work so I needed to decode the values. This is inefficient but fixes what I think was a real bug.

Issues

A few tests are now failing, which are I think related to comparisons with the unicode sandwich comparisons. @taldcroft, could you take a look at the failures since you are the most familiar with that code?

Testing

A number of changes here require their own unit tests, which I can add if we decide to go ahead with this PR, along with changelog entries.

Review

This needs a careful look by the FITS maintainers: @saimn @MSeifert04 @drdavella, as well as Table maintainers @taldcroft and @mhvk, and @AustereCuriosity because of the FITS/Time change.

Also @adrn - please test this out (since your comment on Slack prompted me to work on this!)

(this description was edited compared to its original version)

Closes #6122

astropy-bot · 2017-11-08T19:24:07Z

Hi there @astrofrog 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here.

astrofrog · 2017-11-08T19:41:13Z

+                        # We use [:] but not copy() here so that the new
+                        # col.array is a view of the same region in memory
+                        # but a differe Python object.
+                        col.array = col.array[:]


Unfortunately this doesn't work in the sense that it re-introduces the memory leak :( I'll need to investigate more.

mhvk · 2017-11-08T20:52:09Z

@astrofrog - Is there a way you can check where the large peak in memory usage happens in the data.view(self._data_type) statement? In particular, after taking a view to a new class, that class's __array_finalize__ is called, and FITS_rec has a non-trivial one (see https://github.com/astropy/astropy/blob/master/astropy/io/fits/fitsrec.py#L216, in particular starting L229), including some changes made in #4539.

Overall, I fear this may need a different look at the reference cycle that #4539 tried to address...

astrofrog · 2017-11-08T21:49:53Z

@mhvk - ah good point I can check __array_finalize__

astrofrog · 2017-11-08T22:03:19Z

@mhvk - it turns out the memory increase is indeed in __array_finalize__. More soon!

astrofrog · 2017-11-08T22:21:14Z

Basically it looks like the 2Gb memory appears in this line in __array_finalize__:

            self._coldefs = ColDefs(self)

I'm going to have to put this on pause for a few days or more. If anyone has any time to investigate this further, please feel free to!

astrofrog · 2017-11-08T22:55:08Z

I have some possible fixes coming...

taldcroft · 2017-11-09T11:30:42Z

-    t = Table(table.data, masked=masked)
+    t = Table(masked=masked)
+
+    for col in table.data._coldefs:


It should be somewhat more efficient to make a list of Columns and create t from that.

taldcroft · 2017-11-09T11:39:19Z

            If `True`, image data is not scaled using BSCALE/BZERO values
            when read.

+        - **return_bytes** : bool


I wonder about the name return_bytes. It might not be immediately obvious to code readers what that implies. So maybe string_as_bytes or character_as_bytes (bool) or character_type = ('bytes'|'unicode').

Can we go a bit beyond that and follow numpy in adding an encoding argument (numpy/numpy#4208)? This especially since table columns can now handle at least encoding utf-8 (U would correspond to utf-32, which would be the default; bytes and utf-8 would do the same thing - ideally we change Column to have encoding as an option...)

Maybe try_decode_bytes or try_decode_ascii ? (Only ascii columns are decoded)
Not sure about adding an encoding argument, it can be misleading.

Can FITS columns actually include non-ASCII data though? (regarding specifying an encoding)

I went with character_as_bytes since that is the most intuitive in my opinion

taldcroft · 2017-11-09T11:41:34Z

            raise ValueError("Column {0} does not exist".format(name))

-    def add_column(self, col, index=None, name=None, rename_duplicate=False):
+    def add_column(self, col, index=None, name=None, copy=True, rename_duplicate=False):


Better to maintain API by putting copy at the end.

taldcroft · 2017-11-09T11:54:45Z

@astrofrog - this looks amazing!

The scope of changes within FITS are about what I imagined from the last time I looked at this and looks reasonable. The other changes to Time and Table seem sensible as well.

In real life you need to actually do something with the data. Can you compare timing for these use cases:

Access all of the data for one column
Access all of the data for all columns (in a column-wise fashion)
Access all of the data in a row-wise fashion
Access first 100 rows in a row-wise fashion

By "access" I mean any cheap operation that demands getting the actual value.

taldcroft · 2017-11-09T12:25:03Z

@astrofrog - also I would suggest using a test data file that is at least 2.2 Gb in size. My first attempt at chunked ASCII table reading used mmap (via the fast reader) and it turns out this blew up for files bigger than 2 Gb (which is the 32-bit limit). I was doing this on a modern 64-bit mac and could not find anything 32-bit-ish in my setup, and this problem was the end of that implementation of chunked reading.

Also (sorry) this should be tested on Windows as well with a big file.

astrofrog · 2017-11-09T14:21:42Z

@taldcroft - thanks! I'll do all this :) Would you be able to take a look at the test failures to see if they are related to corner cases in the unicode sandwich comparison stuff?

mhvk · 2017-11-09T14:50:43Z

            # [+/-C]CCYY-MM-DD[Thh:mm:ss[.s...]] where the number of characters
            # from index 20 to the end of string represents the precision
            precision = max(int(col.info.dtype.str[2:]) - 20, 0)
+            if col.info.dtype.kind == 'S':


I'd not be against solving this on the Time side - @taldcroft - what do you think? It would not be difficult (but definitely in another PR).

Actually, col here is an astropy Column, which should sandwich the bytes already, so I think that this doesn't work automatically is a buglet.

See #6823 for a fix. I think it is fairly trivial, so perhaps we can merge it before this PR, so that this change no longer is necessary.

@mhvk - sounds good!

#6823 is merged now.

@taldcroft @mhvk - interestingly I still need these lines otherwise the tests fail, so there must still be some subtleties. Maybe one of you could try and run the tests for fitstime on this branch locally and see what might be missing for Time? In the mean time, I'll leave as-is.

It is a bit strange - it has to do that the sample columns put in that file have different length:

col # <Column name='datetime' dtype='bytes29' length=2> # 1999-01-01T00:00:00.123456789 # 2010-01-01T00:00:00 Time(col[0], format='fits', precision=9) # <Time object: scale='utc' format='fits' value=1999-01-01T00:00:00.123456789(UTC)> Time(col[1], format='fits', precision=9) # <Time object: scale='utc' format='fits' value=2010-01-01T00:00:00.000000000(UTC)> Time(col, format='fits', precision=9) # *** ValueError: Input values did not match the format class fits

the example is not good (col translates to string for items), but the issue is there...

Don't forget to remove this stanza!

mhvk · 2017-11-09T14:57:14Z

Main comment: I wonder if we cannot just use bytes everywhere, since Column sandwiches this anyway. Or is the main worry that people might use view on that and then get an unexpected result?

Beyond being not sure about the name and exact interpretation, this looks great!

astrofrog · 2017-11-09T14:59:59Z

@mhvk - you mean return_bytes should default to True? If that's what you mean then the issue is that this would break things for people using astropy.io.fits directly.

mhvk · 2017-11-09T15:58:17Z

I guess what I'm trying to say is that the moment you translate a fits column to a table Column, there is, as far as I can see, no use in going for dtype U, since Column already sandwiches that. I also thought that was the only place where the conversion was done, but may well be wrong... Anyway, for conversion to Column, I think it makes sense to have no conversion to U be the default, but that can also be done by having Table.read pass on the relevant argument.

saimn · 2017-11-09T15:49:50Z

-    t = Table(table.data, masked=masked)
+    t = Table(masked=masked)
+
+    for col in table.data._coldefs:


Using table.data.columns should be the same here, and it avoids using a private attribute.

Actually even better: table.columns

@saimn - this causes issues I think because of the @lazyproperty on columns. Let me come up with a simple example and open a separate bug report.

I can't reproduce this in a minimal script, but give it a try with this PR: change this to table.columns - in my case this goes from 'reading' the 2Gb table in 40ms to 10 seconds. It seems that in that case FITS_rec.__del__ gets called and there are remaining references to the columns, which then get copied.

(this could be related to the @lazyproperty on columns)

Not completely sure about table.columns (I will check with your branch), but at least table.data.columns is just a property that returns the _coldefs.

I think I managed to get things to work, but it turns out accessing col.array sometimes returns scaled data and sometimes not, so I had to get data[col.name] instead to get around this (see #6837 for an example of this)

saimn · 2017-11-09T16:06:40Z

                    else:
                        fsize = np.dtype(format.recformat).itemsize
-                    return chararray.array(array, itemsize=fsize)
+                    return chararray.array(array, itemsize=fsize, copy=False)


So this is the only real change in io.fits when creating the BinTableHDU object (not taking into account the bytes decoding which happens only when accessing a bytes column). I'm trying to do the same but in my case it seems that the data is still loaded in memory whereas your memory usage report shows that you fully use the memmap. Any idea why, or am I missing something ?

Did you explicitly set return_bytes=True when opening the file? Or you mean creating a new HDU object from scratch? copy=False isn't guaranteed to never copy, it might copy if the item size changes for example - can you share your specific example?

I was trying just reading a file directly with io.fits and looking at the memory usage, independently from Table. And I found the culprit, it's because of a boolean column: just below the chararray block, for the 'L' format the array is read to check if False or 0 values must be converted.

Perhaps worthy of raising as a separate issue; no reason this couldn't be done when truly needed.

astrofrog · 2017-11-09T16:22:54Z

@mhvk - ah ok, I had misunderstood. This PR actually adds Column as S, not U, so i think this already does what you are saying? (and relies on the unicode 🌮 )

mhvk · 2017-11-09T16:25:21Z

@astrofrog - ah, yes, now I see that it was already what I thought made most sense!

For the name, since what we're doing is just skipping ascii decoding, maybe just call the argument decode_ascii? (with a default of True) Or decode_bytes='ascii' (where None` would be no decoding).

astrofrog · 2017-11-29T19:55:33Z

@taldcroft - are the tests I added in dbba587 sufficient?

saimn · 2017-11-29T21:43:44Z

+  ``fits.open``, a new keyword argument ``character_as_bytes`` can be passed -
+  when set to `True`, character columns are returned as Numpy byte arrays
+  (Numpy type S) while when set to `False`, the same columns are decoded to
+  Unicode strings (Numpy type U) which uses more memory. [#6821]


Maybe split in 3 items (performance improvement, memmap, and character_as_bytes ?

saimn · 2017-11-29T21:45:58Z

+        Whether to use memory mapping, which accesses data on disk as needed. If
+        you are only accessing part of the data, this is often more efficient.
+        If you want to access all the values in the table, and you are able to
+        fit the table in memory, you may be better off leaving memory mapping


I'm not sure about this one. I'm asking because I'm curious about the answer, and because the FITS FAQ tells a different story (http://docs.astropy.org/en/latest/io/fits/appendix/faq.html#what-performance-differences-are-there-between-astropy-io-fits-and-fitsio):

In the case of reading over all the data in the file the performance difference between using mmap versus reading the entire data into physical memory at once can vary widely between systems, hardware, and depending on what else is happening on the system at the moment, but mmap almost always going to be better.

If you need all the data, then not using mmap is faster, both with images and tables, for sure. If you need only a small part of the data then mmap is definitely better. In between, it's a gray area. I think that FAQ item is misleading because for cases like reading a table and doing Table.info('stats') then you definitely get better performance without mmap.

From my tests (on datacubes, not tables), when loading all the data, it is faster with memmap. It may depend on the OS and other things, I don't know.

saimn · 2017-11-29T22:09:41Z

The gain is truly amazing, but I would just like to see how it is specific to the type of data that is in your table. With three columns, floats, ints, and 12-character strings, I think the gain here comes mostly from the string decoding ? (And from the copy that was done when creating the Table).
My fear is that the gain will depend on what type of data is in the table, for instance Boolean columns must be converted, or scales must be applied, and the gain may be smaller ? I did not have the time to explore this fully, but from what I saw as soon as your really need to load a column, to convert the data for instance for booleans, the whole array is loaded in memory. Is that possible ?

So, the PR looks fine, I'm just trying to understand better the implications, and to see if it would useful to mention that the gain will be influenced by the data types.
And maybe for future improvements, would it be useful to have other flags to disable conversion for specific data types, like character_as_bytes, or like do_not_scale_image_data which currently applies only to ImageHDU ?

astrofrog · 2017-11-29T22:33:28Z

@saimn - just to be clear, as mentioned higher up, memory mapping is actually off by default here, so the gains are purely due to avoiding unnecessary copies for most datatypes. Indeed by far the biggest gain is due to string columns, which at least for me is a very common use case (e.g. source name). If a column had a boolean type then we might require a copy of that array but that shouldn't slow things down too much since boolean columns don't take up much memory.

The primary goal of this PR was to get rid of the huge bottleneck that comes from string columns, hence my choice of test data. In a separate PR I plan to investigate whether it is beneficial to turn memory mapping on by default and will do more extensive tests as that is trickier.

So just to be clear, with this PR there should be no circumstances under which things will be slower than before. But it doesn't mean there aren't other optimizations that can be made - in fact I have a follow-up PR to improve the writing of FITS tables with string columns for example.

In future it would indeed be good to have more settings to control things like this.

…mns [ci skip]

astrofrog · 2017-11-29T22:51:41Z

@saimn - see the latest commit which clarifies that the performance improvement is in particular when there are string columns.

saimn · 2017-11-29T23:24:53Z

@saimn - just to be clear, as mentioned higher up, memory mapping is actually off by default here

Indeed, sorry for the confusion about this part. And no worries, I agree that it will always be better, I just try to understand better 😉.

saimn

Just a minor comment about the changelog, otherwise this is really great, thanks @astrofrog !

saimn · 2017-11-29T23:26:20Z


- The performance for reading FITS tables has been significantly improved, in
+- The performance for reading FITS tables has been significantly improved
+  for cases where the table contains one or more string columns, in


I think it's useful to mention that the best improvement is for string columns, thanks, but it's not only for string columns. What about "... significantly improved in particular for cases where ..." or something similar ?

I agree - fixed :)

saimn · 2017-12-01T09:55:36Z

Ok, merging now, thanks for the great improvement and looking forward to the next optimizations!

cdeil · 2017-12-02T17:54:54Z

@astrofrog - Thank you for working on FITS Table performance!

I just wanted to mention that this type change of column .data from str to bytes did break some things in Gammapy, where we were relying on getting str: gammapy/gammapy#1228

My understanding is that this is an intentional change in Astropy here for the type of .data: change it from str to bytes and defer the conversion to str to access time, as a performance optimisation for the cases where the user never accesses it?

I'm OK with that, and we can update our code in Gammapy. I just wanted to comment here to make sure the change is intentional.

Would doing the conversion of .data to str on property access be an option? I.e. keep it as type str as it was before?

saimn · 2017-12-05T17:24:52Z

Hmm, @cdeil is right, this is a breaking change. I completely missed that, but the Table unicode sandwich does not decode the data if accessed directly with .data.

Should we use character_as_bytes=False by default then, both for compatibility and because it is more convenient for users to get a unicode array ? Or do you see other solutions, @taldcroft @astrofrog ?

mhvk · 2017-12-05T18:50:50Z

@saimn - this is really related to the unicode sandwich PR itself. My own sense would be that generally use of the Column.data attribute is not going to be common, or is easily worked around, but then again, @cdeil's example can be taken as proving that my sense is wrong...

astrofrog · 2017-12-05T19:22:25Z

I think that uses that depend on Column.data returning bytes rather than unicode are going to be rare and constrained to more advanced users. I would argue the scale of the performance gain is worth it for the average user.

saimn · 2017-12-05T21:36:36Z

Hmm I don't know, I really like performance improvements, but on the other hand:

I guess people are used to use the .data attribute for HDUs (or masked arrays)
Getting the numpy array associated with a column is probably not a so uncommon use case
Getting sometimes unicode and sometimes bytes is really confusing

Just thinking out loud, would it be stupid to have .data returning the decoded array and a .raw_data attribute to get the bytes array ? There was a similar idea for fits iirc, to get the unscaled data.

mhvk · 2017-12-05T22:19:02Z

@saimn - don't forget tables are used by a lot of people who do not read fits files - changing the meaning of data would be an even worse compatibility break!

The one other option that crossed my mind is to use the sandwich only by default for memmap=True (where it makes most obvious sense, since the memory map is only possible using the sandwich). But to me, that risks even more confusion.

Overall, my sense would be simply to warn about this prominently and see where the pieces fall...

saimn · 2017-12-05T23:02:12Z

@mhvk - Yes sure, it was just a thought, but I'm trying to weight the pros and cons ;)

astrofrog added io.fits Performance Work in progress labels Nov 8, 2017

astrofrog mentioned this pull request Nov 8, 2017

Add option for fits table reader to read a subset of columns #6503

Closed

astrofrog commented Nov 8, 2017

View reviewed changes

astrofrog force-pushed the table-fits-nocopy branch from 1350ed6 to 18d556a Compare November 9, 2017 09:57

astrofrog requested review from MSeifert04, drdavella, mhvk, saimn and taldcroft November 9, 2017 10:08

astrofrog removed the Work in progress label Nov 9, 2017

astrofrog requested a review from adrn November 9, 2017 10:10

taldcroft reviewed Nov 9, 2017

View reviewed changes

mhvk reviewed Nov 9, 2017

View reviewed changes

saimn reviewed Nov 9, 2017

View reviewed changes

Fix failure with --open-files

d969d0b

saimn reviewed Nov 29, 2017

View reviewed changes

astrofrog added 2 commits November 29, 2017 22:40

Split changelog entry into three [ci skip]

5d41ba2

Clarify that performance improvement is mostly when using string colu…

d82d653

…mns [ci skip]

saimn approved these changes Nov 29, 2017

View reviewed changes

Improve wording in changelog entry [ci skip]

a92ed0d

saimn merged commit 8ad19c6 into astropy:master Dec 1, 2017

astrofrog mentioned this pull request Dec 1, 2017

Speed up writing of FITS tables with string columns #6920

Merged

cdeil mentioned this pull request Dec 2, 2017

FITS Table string related fails with Astropy dev gammapy/gammapy#1228

Merged

saimn mentioned this pull request Dec 12, 2017

Table creation with FITS_rec works for py2 but not py3 #5298

Closed

saimn mentioned this pull request Dec 21, 2017

Add nocopy argument to (Bin)TableHDU.from_columns #7012

Open

astrofrog mentioned this pull request Feb 21, 2018

Table.read defaults to characters_as_bytes=True #7217

Open

saimn mentioned this pull request May 8, 2018

Table.read on FITS HDU leaves files open #7440

Closed

astrofrog deleted the table-fits-nocopy branch November 14, 2018 15:29

pllim mentioned this pull request Feb 20, 2019

Memory management in Astropy tables #6491

Closed

cdeil mentioned this pull request Apr 12, 2019

Give example for Table.read with memmap=True #8573

Closed

Conversation

astrofrog commented Nov 8, 2017 • edited by taldcroft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Bottom line

FITS changes

Table changes

Time changes

Issues

Testing

Review

Uh oh!

astropy-bot Bot commented Nov 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astrofrog Nov 8, 2017 • 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!

mhvk commented Nov 8, 2017

Uh oh!

astrofrog commented Nov 8, 2017

Uh oh!

astrofrog commented Nov 8, 2017

Uh oh!

astrofrog commented Nov 8, 2017

Uh oh!

astrofrog commented Nov 8, 2017

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!

astrofrog Nov 9, 2017 • 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!

taldcroft commented Nov 9, 2017

Uh oh!

taldcroft commented Nov 9, 2017

Uh oh!

astrofrog commented Nov 9, 2017

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!

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!

mhvk commented Nov 9, 2017

Uh oh!

astrofrog commented Nov 9, 2017

Uh oh!

mhvk commented Nov 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

astrofrog commented Nov 8, 2017 •

edited by taldcroft

Loading

astropy-bot Bot commented Nov 8, 2017 •

edited

Loading

astrofrog Nov 8, 2017 •

edited

Loading

astrofrog Nov 9, 2017 •

edited

Loading

astrofrog Nov 9, 2017 •

edited

Loading