Support reading and writing multidimensional and object columns in ECSV - #11569

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

Hello @taldcroft 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style.

There are no PEP8 style issues with this pull request - thanks! 🎉

Comment last updated at 2021-04-28 17:28:33 UTC

@aaryapatil You asked for review experience. If you want to look through a complex, gold-standard PR to see how everything looks when it's great, but that might still benefit from general thoughts then this one is for you! At least I learned new stuff about astropy and Python by just reading through the code.

@hamogu - your concerns are well-founded. Version one of this #11155 was closer to the idea of all the data being readable as simple CSV. It basically flattened an N-d column into the required number of 1-d columns and then used a column naming convention. After the good discussion in #11368 I came to the opinion that the JSON approach is better.

But backing up, the first branch point is whether to just throw an exception at N-d columns ("can't do it") or "find a way". I think that "finding a way" is not quite an example of that famous XKCD since we are extending an existing format. It has been encouraging to see ECSV getting some traction.

Part the driver for #11368 and this PR is the possibility that the GAIA data processing consortium would use ECSV as an ASCII table format that is compatible with both TOPCAT and astropy. That really made me want to "find a way"! 😄

A distinct need there is to support variable length arrays (ala FITS), and for that the flattening approach just doesn't work. So we came to JSON encoding, at which point it makes sense to do the N-d arrays and object columns using the same JSON encoding. JSON is simple so the encode/decode tends to be fast, and of course it is supported in every imaginable language.

p.s. @hamogu - thanks for the super-quick review!

GAIA is a good use case and certainly much more high-profile than me. How would this get into TOPCAT? IS there coordinated TOPCAT development?
I think some of my concerns can be alleviated by documentation (as suggested above), just so the people don't accidentally write new ECSV that they can't read in other programs any longer. It seems that this scheme is back-wards compatible as in "if you don't have multi-d columns, then I nothing changes". That's good! Also, that means that we don't need (to write and maintain) version-specific separate writers (tab.write(format='ascii.ecsv', ECSV_version=0.9)).

GAIA is a good use case and certainly much more high-profile than me. How would this get into TOPCAT? IS there coordinated TOPCAT development?

@hamogu, the STIL java table I/O library has ECSV support since June 2020 (somewhat stimulated by internal Gaia/DPAC suggestions), and STIL is used for table I/O both in TOPCAT and in GACS, ESA's Gaia archive service software. TOPCAT can therefore read/write ECSV already, and ECSV is going to be be provided as an output option in the next GACS release (soon).

Up till now ECSV has only been capable of scalar-valued columns, while Gaia EDR3 (and later releases) will contain a number of array-valued columns. That discrepancy led the Gaia Archive Scientist to ask about the possibility of an array-valued enhancement to ECSV, and I (Gaia/DPAC member, STIL/TOPCAT author, someone with a small amount of Astropy contact) raised #11368 accordingly.

Regarding future development: once this is written up in the APE, I will implement corresponding functionality in STIL, so it will make its way into TOPCAT and GACS. If there are other documented and language-neutral enhancements to the ECSV format in the future, I'd aim to continue to track them in STIL.

@mbtaylor et al. - here is a view of the output ECSV that comes out of the new code. It is a little unfortunate that ECSV uses the double quote for quoting and that JSON does the same. The ECSV rule is that a single " within a quoted string needs to be represented as "". This results in double-double quoting for multidimensional string arrays. I think that from a parser perspective this is OK since the rules are simple and consistent, but it does impact aesthetics for a human-readable file.

Space delimited

# %ECSV 1.0
# ---
# datatype:
# - {name: a, datatype: string}
# - name: b
#   datatype: float64
#   shape: [2, null]
# - name: c
#   datatype: int64
#   shape: [null]
# - name: d
#   datatype: string
#   shape: [null]
# - name: e
#   datatype: int8
#   shape: [2]
# - name: f
#   datatype: float32
#   shape: [2, 2]
# schema: astropy-2.0
a b c d e f
x [[1,2],[3,4]] [1,2] "[""a"",""b""]" [1,2] [[0.0,1.0],[2.0,3.0]]
y [[1,2,3],[4,5.25,6]] [3,4,5] "[""c"",""d"",""e""]" [3,4] [[4.0,5.0],[6.0,7.0]]

Comma delimited

# %ECSV 1.0
# ---
# datatype:
# - {name: a, datatype: string}
# - name: b
#   datatype: float64
#   shape: [2, null]
# - name: c
#   datatype: int64
#   shape: [null]
# - name: d
#   datatype: string
#   shape: [null]
# - name: e
#   datatype: int8
#   shape: [2]
# - name: f
#   datatype: float32
#   shape: [2, 2]
# delimiter: ','
# schema: astropy-2.0
a,b,c,d,e,f
x,"[[1,2],[3,4]]","[1,2]","[""a"",""b""]","[1,2]","[[0.0,1.0],[2.0,3.0]]"
y,"[[1,2,3],[4,5.25,6]]","[3,4,5]","[""c"",""d"",""e""]","[3,4]","[[4.0,5.0],[6.0,7.0]]"

Source code

t = Table()
t['a'] = ['x', 'y']
t['b'] = np.empty(shape=(2,), dtype=object)
t['b'][0] = [[1, 2], [3, 4]]
t['b'][1] = [[1, 2, 3], [4, 5.25, 6]]
t['c'] = np.array([[1, 2], [3, 4, 5]], dtype=object)
t['d'] = np.array([['a', 'b'], ['c', 'd', 'e']], dtype=object)
t['e'] = np.array([[1, 2], [3, 4]], dtype=np.int8)
t['f'] = np.arange(8, dtype=np.float32).reshape(2, 2, 2)

out = StringIO()
t.write(out, format='ascii.ecsv', delimiter=' ')
print(out.getvalue())

out = StringIO()
t.write(out, format='ascii.ecsv', delimiter=',')
print(out.getvalue())

t2 = Table.read(out.getvalue(), format='ascii.ecsv')

Python implementation detail

When a typed variable length array gets read back in, there are a couple of options:

1. Object-type column of the pure-Python lists that came from json.loads
2. Object-type column of the numpy typed arrays based on type specified by datatype

Right now it does (2), so you get an array of ndarray and typing is strictly maintained. If the ECSV says the datatype=int8 then that's what you see. I think this makes sense but I wanted to note this. Working with arrays of ndarray can be a little fiddly:

In [6]: x = np.array([np.array([1]), np.array([2,3])], dtype=object)
In [8]: x[0].dtype
Out[8]: dtype('int64')   # Expected

In [9]: x = np.array([np.array([1]), np.array([2])], dtype=object)
In [10]: x[0].dtype
Out[10]: dtype('O')  # Huh?

@taldcroft - definitely see your logic, but of course for FITS we also know that in reality there are corrupt headers (e.g., with units not complying with the standard) and we still parse them. But that is the headers, not the data, so perhaps not quite the same.

In any case, quite happy to go with exceptions now - changing from those to warnings if forced to is easier than realizing warnings was a mistake and then trying to change to exceptions!

Separately: ready for a last review?

Sounds good, we'll leave as an exception. I agree it is easier to remove an exception than go the other way.

I see that all tests are passing, so this is indeed ready for final review.

I do plan to squash this down at the end, there was a fair bit of meandering and once this is done I don't see too much value in all the intermediate commits.

FYI see astropy/astropy-APEs#68.

@mhvk - I think I have addressed the last round of comments. 🤞 again on docs and tests (passing locally).

Thanks for the really useful and timely review @mhvk!

And I'll get to work on the What's New.

Might have broken backward compatibility? See #11803

Just for the record, I had to backport one commit from here (6544aa8) to LTS to fix CI failures.