Add MRT format writer to cds.py [GSoC21a] - #11897

This is premature optimization. Columns are numpy arrays and col.max() will do. To account for columns that are fully masked you can do something like:

col.max = col.max()
if np.is_masked(col.max):
    col.max = None

[At least I think I understand the logic of this bit of code. I'm not sure. But that fact alone shows that it might be too complex.]

I think the way this part of the code works is something as follows.

• The max/min values have to be found in column containing Null values.
• If the min/max values themselves are masked, then we want to assign None to them instead of the random masked value (or NaN).
• So, the null values in the column are filled with a large negative number -999999 (a large positive number 999999). If all the values in the column are Null values, then there won't be any number greater (smaller) than this filled value, and thus, the col.max (col.min) value will be set as None.

I looked up this page for more info: https://docs.astropy.org/en/stable/table/masking.html

I am not sure of the way in which these steps above optimize the process, since checking Min/Max will anyway depend on how well numpy functions are optimized. Maybe the default Min/Max checking is time consuming for columns with a very large number of values, in the worst case scenario and the above steps ensure lesser time?

The numpy code itself is probably written in C (much on numpy internal are), so I'm very surprised if this Python is much faster. Maybe it is. The point is that we first want to get a code that working and that we understand. "I think this works something as follows" is not good enough :-) So, I suggest to use a very simple solutions (like my three lines instead of > dozen lines with two different code path) at this point. If there is an if..else we need to write tests for both cases etc. It's just not worth it unless we know that this part of the code is a bottle neck for speed.

Okay. I have already implemented that in _set_column_val_limits() function.
The optimization is probably good only for extremely large tables. The current implementation poses no problems for now.

And then delete the three lines below this. The fact that this test is only one line long also means that we don't need the hasNull attribute at all. Instead, we can go through the code and replace every occurrence of
if col.hasNull with if isinstance(col, MaskedColumn). Not much longer, but save us an extra attribute.
[At least, that's true until we have cases with hasNull that are not instances of MaskedColumn. Not sure aobut mixin columns with e.g. coordinates. So maybe it's better to leave hasNull for now, but the check to set True or False can still be one line only.

Okay. I am saving col.has_null for the time being.

Can you explain to me the logic behind this if-else? I wonder if this could be simplified further, but would be better to first understand what your motive here is.

This is essentially a counter-measure for calculating format and width of columns containing values both in Float (Decimal format) and the Scientific notation. Like in the column s of the following table.

• First, by default, the Float column is designated as having the decimal notation, fformat = 'F'
• Then we go through the column, record by record, to find the exact format and the width required for it.
• If, upon splitting the Float string, it's found that the current value is in the Scientific notation, the column format designation is changed to 'E'.
• If the previous value was in the Decimal format, the column format would have been 'F'. If yes, then the maxSize, maxPrec and maxDec variables are set to default as well. This is done so that the column formatting happens according the value in the Scientific notation, regardless of any other values in the Decimal format.
• If the current value is not in Scientific notation and the column is marked with 'E' (which means one of the previous values was in the Scientific notation), this particular record is simply skipped, in order to give formatting precedence to the value in Scientific notation.

All this helps correctly writing the above table as,

HD81809  1e-07  22.25608   2e+00  67
HD103095 -3e+06 27.25000  -9e+34 -30

With the ByteByByte being,

 Bytes Format Units  Label     Explanations
--------------------------------------------------------------------------------
 1- 8   A8     ---    names    Description of names             
10-14   E5.1   ---    e       [-3160000.0/0.01] Description of e
16-23   F8.5   ---    d       [22.25/27.25] Description of d    
25-31   E7.1   ---    s       [-9e+34/2.0] Description of s     
33-35   I3     ---    i       [-30/67] Description of i       

But we do know the format - we write the data ourselves. We get to decide if we output in scientific notation or not - the relevant Outputter of the class that does the writing will format the numbers. I wonder if we should make use of this knowledge instead of turning it to strings and then inspecting the string to find what function we used to make that substitution.

On the other hand, I also wonder if this is the best use of our time. Since this seems to work, maybe we should move on to other steps (e.g. coordinates) and come back to possible simplify this in the end.

This fails in this case:

t = Table()
t['b'] = [2.123456789, 2e20]
ascii.write(t, format='cds')

================================================================================
Byte-by-byte Description of file: table.dat
--------------------------------------------------------------------------------
 Bytes Format Units  Label     Explanations
--------------------------------------------------------------------------------
 1- 5  E5.1   ---    b       [2.12/2e+20] Description of b
--------------------------------------------------------------------------------
Notes:
--------------------------------------------------------------------------------
2e+00
2e+20

Note that this is the expected behaviour, so that code block is not failing strictly speaking.

If any column has atleast one value in exponential notation, the intent is to convert all other values of the column to this format. The number of decimal places in the integer part is governed by the number of digits after decimal of the original value given in exponential notation. Trailing zeros after decimal are discounted.
Thus your example above. t['b'] = [2.123456789, 2.00e20] would also output the table.

If it is required that the output table has a set number of digits after decimal for all column values, then the original value in exponential notation should have digits, without trailing zeroes, after the decimal in the integer part. So,

t['b'] = [2.123456789, 2.45e20]
ascii.write(t, format='cds')

================================================================================
Byte-by-byte Description of file: table.dat
--------------------------------------------------------------------------------
 Bytes Format Units  Label     Explanations
--------------------------------------------------------------------------------
 1- 8  E8.3   ---    b       [2.12/2.45e+20] Description of b
--------------------------------------------------------------------------------
Notes:
--------------------------------------------------------------------------------
2.12e+00
2.45e+20

This would not be the expected behavior for most users. Silently dropping precision based on an undocumented / tricky rule like this will eventually lead to a bug report if this gets widely used. Consider the very different behavior of two apparently similar cases:

In [18]: t['b'] = [2.123456789, 2.45e15]
    ...: ascii.write(t, format='cds')
    ...: 
Title:
Authors:
Table:
================================================================================
Byte-by-byte Description of file: table.dat
--------------------------------------------------------------------------------
 Bytes Format Units  Label     Explanations
--------------------------------------------------------------------------------
 1-26  F26.9  ---    b       [2.12/2450000000000000.0] Description of b
--------------------------------------------------------------------------------
Notes:
--------------------------------------------------------------------------------
               2.123456789
2450000000000000.000000000

In [19]: t['b'] = [2.123456789, 2.45e16]
    ...: ascii.write(t, format='cds')
    ...: 
Title:
Authors:
Table:
================================================================================
Byte-by-byte Description of file: table.dat
--------------------------------------------------------------------------------
 Bytes Format Units  Label     Explanations
--------------------------------------------------------------------------------
 1- 8  E8.3   ---    b       [2.12/2.45e+16] Description of b
--------------------------------------------------------------------------------
Notes:
--------------------------------------------------------------------------------
2.12e+00
2.45e+16

Ah, I see. Thank you for pointing out this discrepancy in the behaviour.
This can probably be addressed by opting for highest precision.
Would be good to address it separately. So, needs an issue.

Are you trying to create a copy here? You are using the mask-filled columnmcol to create a coltmp Column later. mcol = col could cause issues if you want a true copy.

Nope, copy of the column is not needed. The temporary coltmp is only to fill empty string in place of Null values for String columns, so that col.dtype.str returns proper column dtype.

This large block in the loop should be factored out to a function.

Don't use a variable name like i over such a long context. If someone reading this code sees an i 80 lines later it's hard to know where it was defined and even harder to search for i.

I think i is fine for a counting variable - it's what most people would expect when they see i or j. However, I don't feel strongly about this and would also be OK with a more explicit name like col_num.

Using i is fine for a loop that can be easily seen in one screenful, while in this case the loop blocks are quite long.

While I'm looking at the code, the giant block in the write method could be done without any index variable by making a list new_cols = [] at the top, then looping over columns and processing each column. For the special cases then some transformation of the column happens (possibly turning one column into two or more), but for the rest you just append the original column. Then at the end self.cols = new_cols. This way:

• You don't need an index
• You preserve the original column order. The current code always puts any coordinate columns at the end of the column list.

Yup, I have also thought of doing this.
Since, the output of the tests will change because of different column sequence, it would be better to address this separately. So, needs an issue.

In general, you should never be accessing attributes like col.name or col.description. Those only work for Column and MaskedColumn, but modern "mixin-safe" code needs to always get/set the info attributes like col.info.name or col.info.description. Right now the CDS writer is failing for a simple QTable because of this:

qt = QTable()
qt['a'] = [1,2] * u.m
ascii.write(qt, format='cds')
Title:
Authors:
Table:
================================================================================
Byte-by-byte Description of file: table.dat
--------------------------------------------------------------------------------
 Bytes Format Units  Label     Explanations
--------------------------------------------------------------------------------
1-3  F3.1   m      Unknown [1.0/2.0] Description of Unknown
--------------------------------------------------------------------------------
Notes:
--------------------------------------------------------------------------------
1.0
2.0

Points like this are exactly why we asked you, @taldcroft, to have a look! I'm not using (or writing code for) QTables.

When I check col.name and col.info.name for the qt above, both of them are set to None. Whereas, qt.info() gives out a float64 m Quantity.

This may possibly be because the Basereader.write func in core.py takes the table columns and saves them as a list in header.cols using table.columns.values().

For a Table, t.columns.values() is odict_values([<Column name='b' dtype='float64' length=2> 2.123456789 2.45e+20])
For a QTable, qt.columns.values() is odict_values([<Quantity [1., 2.] m>])

We had not foreseen that QTables may not work with the code. Thank you for your valuable comment. :)
Looks like this may need some work. So, needs an issue.

I think the default description if not provided should just be blank, or perhaps --- or the column name if there must be something there. The Description of ... is just adding noise but not information.

This strongly encourages users to put in a description for every column though, which I think is the intend of the MRT format. MRT tables will (in the use case that we are working towards, although I'm use users will find a way to abuse that) always be edited by journal staff to put in the proper reference information of the published article, so an pretty obvious "PUT SOEMTHING HERE" can be useful. That said, --- fills the same role and is a little less obtrusive.

Since, the output of the tests will change when this is done, it would be better to address this separately. So, needs an issue.

This expression for floats is not correct. E.g. try it with .2 or ++-+-+5 or +hello5. I think the answer https://stackoverflow.com/questions/385558/extract-float-double-value is correct.

Though I guess to be fair this expression might work for parsing the bits of a string that is known to have been generated by "normal" Python formatting of a float value. But do remember that the format specifier for a column can be quite general, e.g. col['a'].info.format = '++{:.2f}'. The current expression would accept values formatted that way, whereas the right behavior would probably be an exception since a float value is being formatted in a non-float output.

So you might test for a correctly formatted float by doing float(value) in a try/except.

This would need some work. So, needs an issue.

This fails in this case:

t = Table()
t['b'] = [2.123456789, 2e20]
ascii.write(t, format='cds')

================================================================================
Byte-by-byte Description of file: table.dat
--------------------------------------------------------------------------------
 Bytes Format Units  Label     Explanations
--------------------------------------------------------------------------------
 1- 5  E5.1   ---    b       [2.12/2e+20] Description of b
--------------------------------------------------------------------------------
Notes:
--------------------------------------------------------------------------------
2e+00
2e+20

I'm confused by this. start_line is not a keyword arg for ascii.write, so I'm wondering what this is doing here?

The data part of the CDS table can itself have header lines, esp. when the data is written in a separate file. The title of the Byte-By-Byte for such tables then says that rows till start_line are header lines.
Because such tables are not ubiquitous, adding the start_line keyword to ascii.write for allowing this feature prob skipped my mind.
Should this be added? Else I can remove that if statement.

I think we leave it in for now.

The Format and Units can both be longer than 6 characters, so I'm confused about the hardwired 6 here.

Yeah, they can be. 6 is hardcoded to have a minimum column width when Format and Units have less than 6 characters. This also helps in the Byte-By-Byte mini-table header row alignment.