I look forward to looking this over in detail when I have the chance :)

For the record here, there I looked at the timing dependence on field width and found some non-linear scaling in the fast branch. In particular if you have a table with floats that are 7 characters wide in total, then go to the full precision case where they are 18 characters wide:

• io.ascii legacy takes 1.2 times longer
• io.ascii fast-c-reader takes 3.5 times longer
• pandas takes 1.8 times longer

http://nbviewer.ipython.org/gist/taldcroft/4ac6e43fad88810c6561

The other way to put it is that for the wide field case, the fast-c version is only 2.1 times faster than the pure python legacy version. For the narrow field case I reproduce your original benchmark that fast-c is about 6 times faster than pure python.

The relative difference between fast-c-reader and pandas may point to some way to get some more speed.

After looking over the conversion code in Pandas, I found it was using a custom string-to-double function called xstrod() which is faster than strtod() from the standard library (which I was using). I just switched to the Pandas function in the last commit, but I'm still unsure whether this is the best solution since there are precision issues:

In [1]: from astropy.io import ascii

In [2]: tfast = ascii.read('float.txt', format='basic', guess=False, use_fast_reader=True)

In [3]: tslow = ascii.read('float.txt', format='basic', guess=False, use_fast_reader=False)

In [4]: tfast['1'][0]
Out[4]: 6.2187331579177556

In [5]: tslow['1'][0]
Out[5]: 6.2187331579177547

The value in the file is "6.2187331579177550499956283", and the fast reader before this latest commit had the same result as in tslow. I ran the code from the iPython notebook and found on my machine that both Pandas and the latest commit in this PR slowed down by a factor of ~2.1 when handling full precision.

Very interesting!

Can you do some quick things to make the results a bit easier to interpret:

• Use an input number like 1.2345678901234567890
• Print the output values with {:.20f}.format(..) for consistency.
• Print the binary version of the output values. E.g.

a = np.array(1.2345678901234567890)
b = a.view(dtype=(np.uint8, 8))
print(b)  # get the raw 8 bytes

• Do the same for Pandas and possibly demonstrate a bug in xstrod() in the "native" context of running in Pandas.

Definitely if this faster converter is not round tripping at the bit level then it can't be used.

Hmmm, it does look like a bug in Pandas:
http://nbviewer.ipython.org/gist/amras1/cbd44004f1a37cfa1529
For now I'll just undo the latest commit, but I'll check what numpy uses for conversion in case they have something that could be adapted here.

Interesting. Have you filed a pandas issue? Maybe someone there will be able to make a quick fix.

I've actually been managing a PR in Pandas which ignores empty/commented lines during reading, so I'll ask about the rationale behind this functionality (or if it's actually a bug)...it does seem like pretty weird behavior.

Minor issue below. This gets me thinking that maybe we don't need the option of use_fast_reader=None, which is currently "use the fast reader if possible". What if we just had:

• use_fast_reader=False: don't use fast no matter what
• use_fast_reader=True: (default) use fast if it exists for that format, otherwise use slow

The latter should be exactly the same in code as the current None option, right? I'm just thinking this simplifies things for users and that the current use_fast_reader=True behavior isn't very useful. Basically it is the same as None except that it might crash if there is no fast reader.

In [1]: dat = 'a b\n1 2'

In [2]: from astropy.io import ascii

In [3]: ascii.read(dat)
Out[3]: 
<Table rows=1 names=('a','b')>
array([(1, 2)], 
      dtype=[('a', '<i8'), ('b', '<i8')])

In [4]: ascii.read(dat, use_fast_reader=True)
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-4-9068e73a7df1> in <module>()
----> 1 ascii.read(dat, use_fast_reader=True)

/Users/aldcroft/git/astropy/astropy/io/ascii/ui.pyc in read(table, guess, **kwargs)
    137         guess = _GUESS
    138     if guess:
--> 139         dat = _guess(table, new_kwargs, format, use_fast_reader)
    140     else:
    141         reader = get_reader(**new_kwargs)

/Users/aldcroft/git/astropy/astropy/io/ascii/ui.pyc in _guess(table, read_kwargs, format, use_fast_reader)
    184         elif format is None and use_fast_reader:
    185             # use_fast_reader=True, but the user did not specify a format
--> 186             raise ValueError('The parameter "format" must be specified in order to use '
    187                              'a fast reader')
    188         elif use_fast_reader:

ValueError: The parameter "format" must be specified in order to use a fast reader

In [5]: ascii.read(dat, format='basic', use_fast_reader=True)
Out[5]: 
<Table rows=1 names=('a','b')>
array([(1, 2)], 
      dtype=[('a', '<i8'), ('b', '<i8')])

In [6]: ascii.read(dat, use_fast_reader=None)
Out[6]: 
<Table rows=1 names=('a','b')>
array([(1, 2)], 
      dtype=[('a', '<i8'), ('b', '<i8')])

Ah--when I first added use_fast_reader I imagined that use_fast_reader=True would be rarely used, except in speed-critical scenarios (e.g. if a file takes a long time to load with the legacy reader and the user would rather have the fast reader fail than wait that long). Actually this is sort of a different issue, since what you noticed was a case in which no fast reader was available...I guess another possibility would be to retain use_fast_reader=True (to explicitly specify not to fall back on the slow reader if the fast reader fails), but change the functionality so that ascii.read() will ignore the parameter if no fast reader is available.

Update on the string to float conversion issue: it seems that xstrtod() was designed simply with speed in mind, and being off by more than 0.5 ULP is considered acceptable since it's beneath the usual precision of floats. Seems a little strange...

After looking around, it seems there isn't a way to convert a string to a double quickly while staying within 0.5 ULP (e.g. strtod() has to go through a correction loop if the number of significant figures is too high). numpy ultimately uses PyOS_ascii_strtod(), which seems to be pretty similar to strtod(); out of curiosity, I tried switching to numpy's default conversion by indexing the output ndarray rather than its underlying double * pointer. Using the same setup as the notebook above, the results were mixed:

In [4]: %timeit fh1.seek(0); ascii.read(fh1, format='basic', guess=False, use_fast_reader=True)
1 loops, best of 3: 1.29 s per loop
In [5]: %timeit fh2.seek(0); ascii.read(fh2, format='basic', guess=False, use_fast_reader=True)
1 loops, best of 3: 512 ms per loop

The old way (using strtod()):

In [5]: %timeit fh1.seek(0); ascii.read(fh1, format='basic', guess=False, use_fast_reader=True)
1 loops, best of 3: 1.08 s per loop
In [6]: %timeit fh2.seek(0); ascii.read(fh2, format='basic', guess=False, use_fast_reader=True)
1 loops, best of 3: 691 ms per loop

So it seems numpy's conversion is better for narrow precision, but my guess is this can vary quite a bit. Interestingly, I ran your IPython notebook on my machine (with strtod()) and found that for the 6-column table, the slowdown in going from narrow fields to full-precision fields was:

• 1.2 times slower for the legacy reader
• 1.6 times slower for the fast reader
• 1.8 times slower for Pandas

I wonder why strtod() varies so considerably...

@AmrAS1 - with regards to the Pandas xstrtod() function, do you have a feeling for the worst case error in conversion? Is it less than 1.0 ULP?

@taldcroft - Hm, I'm not sure. So far I haven't seen a case in which xstrtod() and strtod() differ by more than 1 ULP, but of course that doesn't guarantee that xstrtod() is actually within 1.0 ULP of the correct result. I'll check it out.

Looks like xstrtod() has no worst-case error bound, unfortunately. I tested it out with 20-decimal values and got the following results:
http://nbviewer.ipython.org/gist/amras1/3883ed6caa2c2efa326a
74% of values came within 1.0 ULP (and about 96% within 2.0 ULP). 53% came within 1.0 ULP for 30-decimal values, so the accuracy drops off with the number of digits. This definitely looks like something to document for use_fast_converter=True.

@AmrAS1 - Very cool! I spent a little while playing with this because it seems pretty important to science-oriented folks that might actually care about ULPs. To understand things fully and make life simpler I ended up changing the random number to be 1.xxxxx instead of 0.xxxxx so that the ULP is basically always the same at around 2**-52. Otherwise you can randomly get strings of 0 at the beginning and end up with a smallish number. The way you did everything with Decimal is clever and makes everything work just fine even in the leading-0 case, it just took me a little longer to understand why. 😄

Also it seems like there was a possible uint8-rollover case that wasn't being handled. This only showed up when actually looking at the ULP values, but not in your final histogram. The way I got around this is a bit of a kludge.

http://nbviewer.ipython.org/gist/taldcroft/c0034f876604673e62e8

Great plot, something like this should go in the docs (and maybe into your Pandas issue!).

I'm not going to have time to look at this in any detail, but it looks fantastic, nice work @AmrAS1!

One question: is there a reason to keep the "old" readers around at all if these work better for both reading and writing? That is, maybe there should be no "fast" and "slow" distinction at all, just replace the existing ones wholesale? (Or perhaps move them to the docs as examples but not be in the code itself anymore)

@eteq - Thanks!

My guess is that there are probably a couple corner cases I haven't yet discovered in which the old and new readers perform differently, since there are so many cases to deal with in the C tokenizer. I think it would be okay in most cases to replace the old readers with the new ones, but maybe some file out there can be read only with use_fast_reader=False. The ascii reader always tries using the fast reader first whenever it can, so I don't think there's a downside to keeping the old readers (except that they might mask possible issues with the fast readers).

I guess this is related to the issue @taldcroft brought up previously, which is whether use_fast_reader should have an option to explicitly choose between the fast and slow readers. One problem is that an exception results if the user specifies use_fast_reader=True with a non-fast format--I'm wondering if the ascii reader should silently ignore use_fast_reader in this case.

Ok, that makes sense. I agree that it makes sense for use_fast_reader=True to mean "use the fast reader if there is one, otherwise the slow one". You might consider changing the name to try_fast_reader or use_fast_if_possible or something, though.

@AmrAS1 - In the current scheme, the default for use_fast_reader/writer is None. It seems like all the pytest parameterize selections should include None as an option in addition to False and True. Does that make sense?

Yep, good idea -- I just pushed a commit adding None to the test fixtures.

@eteq and anyone else listening, on our weekly tagup we decided eliminate the None default in favor of:

• use_fast_reader=False: use the legacy Python reader
• use_fast_reader=True: use the fast reader if possible

Same applies for use_fast_writer. One of the possibilities this eliminates is the "I have a huge file and must insist on using the fast reader", but we realized that the user can still do this by specifying format='fast_basic'. Overall this scheme seems simpler and we'll put in documentation to this effect.

Since the default is true, most of the time you would see this keyword in code would be like:

# Use legacy Python reader for stability in our production code
t = ascii.read(my_weird_file, use_fast_reader=False)

In this case the keyword name is spot-on and accurate.

The multiprocessing branch is currently passing on Travis and seems pretty stable, so I just merged it into this PR. Once I change the behavior of use_fast_reader I'll revert the addition of None to the test fixtures.

Great, thanks for the update, @taldcroft. Sounds good to me.

This latest commit should fix the speed issue @taldcroft noted with FastWriter ending up slower than the ordinary writer for custom-formatted values. I ran the new code in the IPython notebook for writing performance on my machine with the following results:

http://nbviewer.ipython.org/gist/amras1/91b3a4129517009b2765

The fix is a bit of a hack even though it works fine for now, so at some point it'll be worth looking into #2823 for a more general solution.

Now that the multiprocessing branch has been merged and I fixed up a couple speed issues (particularly in string conversion and tokenization), I think the analysis using the IPython notebook @taldcroft wrote for reading is basically final. I modified it slightly and ran it again with these results:

http://nbviewer.ipython.org/github/amras1/ascii-profiling/blob/master/ascii_read_bench.ipynb

The notebook now uses a temporary file instead of StringIO (since the fast reader otherwise stores the input in memory as a huge string, which makes it slightly slower) and includes use_fast_converter=True. One interesting result to note is that Pandas performs quite a bit better than the fast reader for repeated string values, but is almost twice as slow when the string values are randomized. This is because the conversion approaches are different; Pandas stores string columns using dtype=np.object_ (obviously inefficient) but uses a C set of PyObject * pointers to avoid repeating values, while the io.ascii fast reader uses np.str with fixed size calculated by finding the maximum field length during conversion. I'm not sure how common it is for text columns to contain a lot of repeated values, but anyway I plan to include this in the documentation.

Apparently IPython doesn't work well with multiprocessing, so I couldn't get reliable results by adding parallel=True to the notebook. Instead I wrote a script (using parts of the notebook) which outputs local results to images and an HTML file. It definitely seems that parallel=True is more effective the larger the input is, so shifted the maximum number of rows in the trial run to 200,000 and got these results on my laptop (use_fast_converter=True is enabled for both serial and parallel reading). Of course the results of parallelization will vary somewhat, so I'm not sure how much of an advantage it is on different machines.

Fastest reader in the West! This is just awesome @AmrAS1.

@taldcroft - Thanks!

Looking over one of Wes McKinney's blog posts, it seems the rationale behind storing strings as Python objects is the memory advantage. This brings up the issue of memory efficiency, which unfortunately the fast reader doesn't seem so hot on; a sample run of the heap profiler Massif on a 10,000 line float file indicated that the new reader takes up far more memory than Pandas, and in fact about 25% more than the legacy reader. I'm pretty sure this has to do with the way the C tokenizer stores output data, so the only way to change this is to restructure the tokenizer. Since the main priority at this point is to write documentation and make sure everything works correctly, I'll table this (no pun intended 😄) for now and hopefully find a solution once everything is completed (or in a separate PR).

@astrofrog - In fact the fast readers / writers are the new default, but there is a fallback to the original readers if any exception occurs. So we do need to be careful about this, but the test fixtures have been updated so that essentially all of the (relevant) existing tests are now being run with both fast and slow readers. For a change of this complexity we're really relying on the tests, so that's probably the area to focus any final review. Before merging I'll give that another scan and look for any holes.

@taldcroft - ok, sounds good! But since as you said there is a fallback to the original readers, this should be safe (except against segfaults of course). But in any case, the sooner we merge, the more testing will happen before 1.0 :) I'm too short on time to review the full code here but as long as @embray and/or @mdboom have had a chance to look at this, then I'm fine with it.

@embray - I opened a couple of issues to capture your two open comments.

This latest commit fixes the issue @embray noted with calls to FastBasic.__init__(). I like the idea of adding those two concerns as separate issues--hopefully I'll be able to address those once this is merged.

One minor item: It's not obvious to me what the defaults are for fast_reader. That is, if I do fast_reader=True, is use_fast_converter on or not? I'm not sure if this belongs better in the docstrings, narrative docs, or both, but it should be somewhere (or if it is somewhere, than maybe it should be in more places because I couldn't find it easily).

Also, perhaps we should add a "What's new" section about this? It certainly shouldn't hold up this PR if it's going to take any significant amount of time, but at some point it's certainly worth "headlining" this as a major improvement that will come in 1.0. And it's fresh in @AmrAS1's mind now, so it might be a good time to write it. (Unless @embray or @astrofrog think we want to only do what's new stuff right before release?)

Other than that, I'm happy with this and am 👍 to merge!

Looks like that's mentioned once in the docs, but I can add a notice in docstrings somewhere as well. As for the "what's new" addition, should I just begin a new file with an empty "overview" section and just write a section on fast/reading writing for now?

I would propose holding off on the What's New section until a little closer to the next release. That will give this update a chance to get used and to sink in, and it's possible there will be tweaks in the meantime. Also this will be one of the bigger highlights so we'll want to take a little time to make sure the text is good and gets properly reviewed.

It looks like everyone is agreeing that this is ready to merge, which I would like to do tomorrow afternoon if nobody else has any concerns.

@AmrAS1 - I'm merging now! I will fix the doc string mistake in master.

Oops, must've been a copy-paste error. Thanks for the catch!

Thank you @AmrAS1 for the great contribution!

@astrofrog - you're welcome, glad to have this merged at last!

Typo fixed in c027ce8.

@AmrAS1 @taldcroft - sure, delaying What's new is fine too - just want to make sure it's in everyone's head as a feature we'll want to highlight there!

And the change @AmrAS1 made in ui.py for 81e0742 is exactly what I was thinking. Glad to see this all made it in!

@eteq - I've started a list for the what's new 1.0: #2935