io.ascii.cds does not implement some aspects of the CDS missing value specification - #1335

I'm not 100% sure how to interpret "when there is a string of ". Would that be a regex like $<blah>+^?

That's how it is in this table and that's how I understand their answer.
I would be fine if we implement it this way - it'll be backwards
compatible, very unlikely to brake anything and solve a problem I
encountered in practice.

(I know it should be possible to read through their fortran code to see
how they do it, but I refuse to learn another language just for this
purpose.)

I've encountered the same (or related) problems in several other CDS tables now, e.g. in table 1 and 2 of:
http://vizier.u-strasbg.fr/viz-bin/VizieR-3?-source=J/A%2bA/519/A34/tablea1&-out.max=50&-out.form=HTML%20Table&-out.add=_r&-out.add=_RAJ,_DEJ&-sort=_r&-oc.form=sexa

Table 1 form the link above has a related problem. The code for fill values is ?, but the actual fill_values are --, so the CdsReader expects the following fill code in the ReadMe file (it works fine, when I edit the ReadMe manually): ?=--. This time I did not correspond with the helpdesk to confirm that this is the expected bahavior, but given that there are apparently many tables on CDS that require a more flexible definition of the fill values, we should probably allow the fill value -- for all numeric columns that have a fill marker starting with ?.

After all, we don't want to write a CDS table verifier, but a tool that reads all CDS tables hassle free (even if they may not correspond to the published format, but only to the format read by the published fortran code).

@taldcroft
I did not take the most general approach (using regex), but a simple one. So far, I have only seen this type of problem for numbers that are marked with ---. The fact that --- for the value to be replaced was hardcoded if - is given in the column description tells me that this is also the only case that was in encountered in previous versions and probably the only one CDS uses.

I don't understand what the Travis fails means... advice?

@hamogu - thanks, this looks generally good. The Travis fail is just a random occurrence, probably resource related. When you push up changes in response to my comments then hopefully it will go all the way through with no fails.

I don't want to change the fill_value=-999. So far, nobody reported problems with it and even if only part of it is applied, it's not too bad (-99 is still a common marker for missing values, so is "-". We can argue about "-9"). In any case, the fill_value is only interesting if somebody requires masked=False otherwise they will get a masked table and any change of that would have to go through a depreciation release.

This is mostly a left-over from asciitable, I think, when users could run this without having numpy installed.
We could consider removing the entire filling thing and just return a masked table in a case with missing value with no option for a non-masked table with filled values (just use table.filled() after if you don't want that). I will think about that and open an issue if I think that makes sense. It's beyond this PR, though.

For this CDS file the current fill_value=-'999' does the wrong thing:

================================================================================
Byte-by-byte Description of file: datafile3.txt
--------------------------------------------------------------------------------
   Bytes Format Units  Label  Explanations
--------------------------------------------------------------------------------
   1-  3 I3     ---    Index  Running identification number
   5-  6 A2     ---    RAh    ?=-- Hour of Right Ascension (J2000) 
--------------------------------------------------------------------------------
  1 03
  2 --

In the following the output dtype for RAh is S4 instead of S2 because of the -999 string that was injected and then immediately masked.

In [1]: from astropy.io import ascii

In [2]: ascii.read('cds.dat')
Out[2]: 
<Table rows=2 names=('Index','RAh')>
masked_array(data = [(1, '03') (2, --)],
             mask = [(False, False) (False, True)],
       fill_value = (999999, 'N/A'),
            dtype = [('Index', '<i8'), ('RAh', 'S4')])

What you list above is not a bug, that's a feature :-)

OK, seriously, I thought we would still (as we did in asciitable) support reading an ascii file with fill_value value, where the value in question would be replaced and not masked. If that is the case, then "0" would be a very bad fill value for RA, "-99" would be much better (even if it changes the format). Now, as I look for it, I don't see the option that would allow me to read an ascii table with missing values and not get a masked table back.
Can I still do that or did we drop that when asciitable was integrated with astropy?

If there is no way to "replace" instead of "mask", then all that matters for a fill value is that it fits the format of the colum, because we never look "behind the mask". That also means that the in
<fill_spec> = (<old>, <new>, <optional col name 1>, <optional col name 2>, ...)
(see http://docs.astropy.org/en/latest/io/ascii/read.html#bad-or-missing-values)
is almost entirely pointless, because that only sets the value behind the mask.

I say "almost entirely" because all that matters is that the value has the same type as the remaining column, e.g. float. However, there would be no reason for the user to set it, we could just find the datatype of the list of non-masked entries and use that as column value.

(However, changing the definition of the fill_value would be a major API change and we need to discuss if we want that and how we handle it separately.)

Can I still do that or did we drop that when asciitable was integrated with astropy?

We dropped that. If any values match the fill_value <old> then the returned table is always masked.

I say "almost entirely" because all that matters is that the value has the same type as the remaining column, e.g. float. However, there would be no reason for the user to set it, we could just find the datatype of the list of non-masked entries and use that as column value.

Exactly. That's why we use ('', '0') for the default because '0' matches any data type (except S0, but that is truly a useless corner case). So in the near term, without trying to solve the bigger problem of the current fill_value spec being a bit over-specified, changing the fill_value in CDS would solve the one problem I showed.

I think in the asciitable to io.ascii transition we left the fill value stuff exactly for backward compatibility.

One thing that has been nagging me forever is the lack of support for automatically setting the numpy masked array fill_value. There are three things we deal with:

• null_value: the string in the original file that is a sentinel for missing data (aka <old>)
• replace_value: the string that gets put in the column str_vals list as a filler so parsing gives the correct type. The value doesn't matter because it is always behind the mask. (aka <new>).
• fill_value: what t.filled() will give as determined by the column fill_value attribute.

When reading an ASCII table there is no way to specify the fill_values. It's probably confusing for people that we have a fill_values arg that has nothing to do with fill_value in the output.

@taldcroft Travis passed for the previous commit and now fails again with something totally unrelated. I set the default to '0', but did not add tests to check if we change any of the column width. Before we do that, we should decide if that is actually a required feature (tests) or just nice to have (no tests). Since not every format expression that could occur in CDS tables has an exact equivalent in python, I don't think we should require that all formats are exactly the same (e.g. what about float precision on 32 vs 64 bit systems - they might differ). If you want, we can add the test for the lengths of string columns.

Looks good to me, merging.

It is an old story... CDS usage asks a specialized understanding.. I expect that it would be improved with the renewal of documentation !
I just make a test (astropy 4.0.1.post1) and I confirm that multiple '-' works when declared with ?=-
Thank you to have fix the issue!