Raise exception when fast_reader called with incompatible formats, guess all available fast formats - #5578

Well, appears MSVC users had some of that functionality all along...

strtod expects nptr to point to a string of the following form:
[whitespace] [sign] [digits] [.digits] [ {d | D | e | E}[sign]digits]

I've revisited the speed tests in the ascii_read_bench notebook to investigate the performance penalties; as expected for optimum performance with all table sizes you should still turn guessing off.
ascii.read(format='basic', guess=False):

ascii.read(guess=True):

What's the "Fast converter"?

In the tests above, it's with the fast_reader=dict(use_fast_converter=True) options (and actually also with the new exponent_style='Fortran' option, but the exponent handling does not seem to have a noticeable performance impact).

okay, then what is "io.ascii Fast-C"? Is this the same but with not-fortran-style exponents?

No, with use_fast_converter=False, i.e. using the stdlib strtod() parser instead of the optimised xstrtod(). exponent_style is an additional option available for the latter (see #5552 for details).

The notebook needed a couple of updates to get it to work with the current master:
ascii_read_bench

So, this PR has evolved beyond the original intend of just raising errors to fix test failures in parallel mode, is my interpretation correct?

The original issues in #5574 were not specifically related to parallel mode, but more generally to requests of fast_reader-specific options when actually a slow reader was called. The latter turned out to happen much more often than expected, so yes, a large part of the PR now is avoiding these fallbacks to the slow reader. This includes that _guess will now in most cases find the correct fast reader.

Not being familiar with the functionality myself, is it correct to assert this as an API change that effects released version? Just need to know for tagging and milestoning. Thanks!

Not sure if it qualifies as an API change; ascii.read() will now stick with the fast reader in a number of cases where it would have fallen back to the slow reader previously. The only cases where this should change results are for the reading of Fortran-formatted data just introduced in 1.3. E.g. in the released version only space-separated ('basic') formats would have been correctly detected:

>>> ascii.read("a b\n0.12d0 2.d3", fast_reader=dict(exponent_style='d'))
<Table length=1>
   a       b   
float64 float64
------- -------
   0.12  2000.0
>>> ascii.read("a,b\n0.12d0,2.d3", fast_reader=dict(exponent_style='d'))
<Table length=1>
  a     b  
 str6  str4
------ ----
0.12d0 2.d3

whereas now

>>> ascii.read("a,b\n0.12d0,2.d3", fast_reader=dict(exponent_style='d'))
<Table length=1>
   a       b   
float64 float64
------- -------
   0.12  2000.0

The only cases where this should change results are for the reading of Fortran-formatted data just introduced in 1.3. E.g. in the released version only space-separated ('basic') formats would have been correctly detected

Thanks for the clarifications! This sounds like a bug fix, so I'll tag is as so for now. Subpackage maintainer can re-tag as appropriate.

Yes, I'd certainly label it as a bug fix (even where the results were not changed, in some instances it would silently fall back on the slower reader). Would still be interesting to know it it in fact solves #5693 as well, or if there is more work to do.

Pinging @olebole in regards to testing this for #5693

@plim That is difficult since I have no direct access to the framework that produced the error -- it is run only on our unstable and testing areas, which are the official and proposed packages. Because of our upcoming freeze, I don't want to put that into unstable just for a test.

Given the previous comment, I'm re-milestoning this to v1.3.2.

@bsipocz, if I understood @olebole right, it is now merged into Debian unstable and passes the tests.
So all we do not know for sure at this point is, if this was actually this PR that fixed #5693, or if the error simply did not show up because it only appears erratically.
Not sure what else is left to do here, except perhaps construct a much larger and more complex table to the test that might force the test failure to show up.

@taldcroft - It seems that I've managed to rebase, pushed the result to my fork to be sure that travis passes. (The diff be viewed here: https://github.com/astropy/astropy/compare/master...bsipocz:dhomeier_fortranexp2?expand=1, travis is running here: https://travis-ci.org/bsipocz/astropy/builds/377092449)

If you're happy with it, I can force push it to this branch.

Thanks @bsipocz; I assume the download failure in the coverage tests is unrelated.
I have two more commits adding a test for the fast_reader dict staying intact (failing on master, passing here) and an update to the docs. They are simple additions to test_read.py and read.rst, so should be trivial to merge either before or after you've pushed your fork - just let me know when to push mine.

@dhomeier - Please add those commits to here. I can always cherry pick those commits, but would rather not force push the rebase without @taldcroft's approval.

@bsipocz - thanks!! It looks like there are a few diffs that I don't entirely understand between your rebase and the last commit when you did the rebase (before the last two), i.e.

git diff dee6318 dhomeier_fortranexp2

However, these are all just trivial formatting / whitespace diffs so I am 👍 doing the force push, then cherry-picking the remaining commits. (And possibly doing a final clean-up to re-apply the format/whitespace diffs).

@taldcroft - Indeed, those whitespace stuff were rebase remnants from an earlier commit. I've cleaned them up in the last cleanup commit.
To play it safe a copy of the original wast kept in the dhomeier_fortranexp2_backup_original branch, the only remaining diff is kept to be more consistent with the locally preferred line length.

I don't know of any outstanding issues left.

@dhomeier and @bsipocz: good for merge?

@taldcroft - yes, I think it's good to go.

@taldcroft, seems I cannot view the current changeset after the forced push, but from my last commit and your cleanup everything looks good to me, too.

🎉
Thanks @dhomeier and @bsipocz !

🎉

Thanks @dhomeier for the contribution and your patience!

Adding to the list, this really was a concentrated effort by all three of you, @dhomeier, @taldcroft, and @bsipocz. The PR would not have been finished in this form if any of you had not put in the large effort that you did to finish this.

It's a testament to the complexity of our ascii reader classes that it needed that much effort, but thanks to you be can continue to improve the performance and consistency to make them even better.

Thanks @taldcroft and @bsipocz for helping to get this PR into a mergeable state, this was indeed a task that had grown beyond my means!