io.ascii fast C converter support for Fortran formats and extended precision - #5552

Since this seems to be working now... @taldcroft, are you happy with it? I give it a quick look, and while I didn't dig deeply into the C code, it looks good to me. Should we squeeze it in for 1.3?

@dhomeier - One thing that's missing is a changelog entry: you'll need to add a section under 1.3 -> "New Features" ->astropy.io.ascii , you'll need to add a sentence or two describing this addition.

Actually, @dhomeier, here was a surprise:

ascii.read('filename', fast_reader=dict(exponent_style='D'))

(where filename is a table with "1.23D4" style numbers) - that silently failed (giving str columns), even though this works

ascii.read('filename', fast_reader=dict(exponent_style='D'), format='basic')

perhaps you should catch combinations where exponent_style is not 'E' and raise an error so the user knows that's an invalid combination?

Thanks for catching, this; in fact I had some trouble with the fact that the fast_reader generally only works reliably with the format specified, and optimally even guess=False set. This is noted in the io.ascii docs, since read() will try a number of formats without fast_reader implementation first, and in general returning a str column is not considered an error.
Of course you have a point that one could expect an error being raised here instead of silently moving on to strings, but I think the situation is not as straightforward - there can be legitimate cases of mixed data files with both string and float columns.
I think the best resolution would be to modify the list of formats tested by guess whenever fast_reader is specified, to have the non-fast formats tested last, or even not at all (there is a point for raising an error when fast_reader has been explicitly asked for but cannot be used on that file)...

@dhomeier - thanks for getting back to this!

@eteq - about pushing for 1.3, I'm just a little hesitant about rushing this now. The code changes are complex, and I don't know if anybody else actually has read and understood the patch. That's partly my fault, but to this day I'm not an expert on the fast C reader code. Recently @saimn appears to be learning this code so maybe he might be able to have a look?

Another strategy is to trust the new code by testing, but honestly I have had trouble looking at the tests and deciding if they are complete. So here is a comment that I had earlier in #3655 that might help:

Overall it is difficult to look at the tests and get a good understanding of what is being tested, in particular with regard to what parses and what does not. What would help is defining this in terms of a uniform test matrix like:

text_value expected exponent_style
...

You could actually do the same in code by just making a big list of tuples with those same three value expected exponent_style. The point is that the coverage of various format possibilities (correct and incorrect) is crucial here. It will be much more obvious when organized in a single table.

@taldcroft - thanks for the feedback! Not sure I understand 100% how you envision the implementation of the test matrix, but I have adopted some of your earlier suggestions into test_data_out_of_range and test_fortran_invalid_exp, only with the input fields - expected values lists horizontally stacked.
I've tried to give test_fortran_reader a bit more structure by iterating over a dictionary of all enabled styles and valid exponents, but I am hesitant to test everything against a single row table, because cases where scientific notation entries only show up somewhere down the table should also be tested.
But I see it would probably be a good idea to also add a dictionary with invalid (= fallback to str) formatting combinations.

@dhomeier - I guess what I mean is this: If I do fast_reader=dict(exponent_style='D') as a user, I'm saying "don't use the default, I specifically want the exponent_style stuff to work". So if I've made that active choice, I shouldn't have it silently fail. Note that I actually tried this with guess=False and even then it silently succeeded, which was very confusing. I think a lot of users will get tripped up by this. So I'm saying that if the Reader does not support a non-default exponent_style and the user has overridden the default of exponent_style='E', an exception should be raised.

@taldcroft re: 1.3 - Other than the above, I think this is conceptually straightforward enough that even if there is a bug, I think it's better to get it in for live testing. (Partly I'm influenced by just having a couple people ask me about this feature in the past few weeks. It also turns out to be a feature Pandas is missing...) I'd be concerned if it were likely to cause regressions, but in my review I was mainly looking for that and didn't see evidence that that was at all likely. (we can have a discussion on this at the coordination telecon today if you like)

So I'm inclined to say try to get it in for 1.3 but also hopefully @dhomeier can try to add some tests as you suggest?

Recently @saimn appears to be learning this code so maybe he might be able to have a look?

@taldcroft - I had a look, but I'm afraid I cannot help you much here ... Overall it looks good, but there are some parts that I don't understand, and it's difficult to get the wide picture.

@saimn - thanks for having a look.

@eteq - completely taking your point, there is already a check raising an error if exponent_style is set in combination with use_fast_converter=False. I just haven't found my way through the guessing mechanism to see where to test for exponent_style='D' before any of the non-fast readers are tried. And I still think this should be tested for the other fast_converter settings to avoid having another format being selected first, because even if the returned values did not change in such cases (that I am aware of), the user would experience a performance hit without immediately noticing.
It looks like #3418 could provide a starting point for adapting the guess sequence...

Re: your remark
'Note that I actually tried this with guess=False and even then it silently succeeded, which was very confusing.'
are you saying you used exponent_style='D' with guess=False and e.g. format='basic' and still had str returned? If so, could you provide details, because this is a failure I have not seen yet?

@dhomeier - I used the following table:

a b
1 4.245D8
2 4.241D-2

And that gave me the following:

>>> t1 = ascii.read('testtab', fast_reader=dict(exponent_style='D'), guess=False)
>>> t1['b'].dtype
dtype('<U8')
>>> t2 = ascii.read('testtab', fast_reader=dict(exponent_style='D'), guess=False, format='basic')
>>> t2['b'].dtype
dtype('float64')

Why didn't it work in the first case, despite me specifically asking for an exponent_style? So what I'm asking is for the machinery to be smart enough to raise an error for those formats that don't support exponent_style if it's anything other than the default (saying something like "exponent_style other than the default is not supported by reader X"). Then you don't care at all about the guess order, but rather you can have all those that don't support exponent_style fail, as they should if the user asks them to do something they can't handle.

(@taldcroft might have some insight to the above question?)

Oh, and FYI, @dhomeier, we discussed this at the coordinators meeting today and we want to try to get this in for the feature freeze/1.3, but with an understanding that we need to be extra-vigilant on this because it's difficult for folks to review the C code of the parser given its complexity. So if it turns out this introduces a bug we missed and it's revealed in testing the RC, we might pull it back out or something.

But if you can resolve my one lest question above, and add a changelog entry, I think this is good to go!

@eteq - thanks for the positive feedback; of course if any bugs remain to show up this should not make it into the release!
I agree it is certainly best practice to have the conflict with unsupported fast_reader options raise an error directly at the reader level, or at least directly when calling that reader. Just thought it might be easier to handle this in a more central location like _guess instead of having to ensure that readers for all possible formats (and potential future additions) follow this rule, but the latter is probably the more robust solution. Unfortunately this is not made easier by the way read() arguments are obfuscated on their way down to the actual parsers, e.g. core._get_reader() simply strips all fast_reader keywords if called for a reader without fast implementation, before proceeding any further. So this probably should raise an error as well?

Table.read(['====== ===', '    a     b', '====== ===', '3.14e4  42', '====== =='], format='ascii.rst', fast_reader='force')
<Table length=1>
   a      b  
float64 int64
------- -----
31400.0    42

Maybe it just takes replacing

    if 'fast_reader' in kwargs:
        del kwargs['fast_reader'] # ignore fast_reader parameter for slow reader

with an Exception in io.ascii.core; will give that a try.

@dhomeier - I took a shot at this (with a test) in https://github.com/eteq/astropy/tree/fortranexp2-fast-error , but it doesn't seem to work...

But this does make me realize that the underlying problem is more than just exponent_style - it's inherent to the fast reader itself. So I think that's beyond the scope of this PR... With that in mind, I'm going to go ahead and make a changelog entry (although in the future please do that yourself with new PR's), and merge this. Thanks for your contributions!

(And I'll create a separate issue about this fast_reader confusing business to keep track of that).

@eteq - thanks, but could you hold it for another hour or so - I had some (hopeful) clarifications to the tests I had not committed yet.
Ah, OK, too late.
Will see if I can add those in a follow-up PR; sorry I had left CHANGES.rst pending while it was not clear if this would go into 1.3.
About the problems with _get_reader - I realised generally raising a ValueError would not work, since fast_reader ist True by default.