Update astropy.io.vo to use the astropy.table.Table class - #363

Thanks for filing this. I'm aware of the issue, and have taken a kick at this can a few times. The main sticking point is the mask functionality. Numpy's masked array functionality alone is not able to mask individual elements of individual table cells (i.e. if a cell itself is an array, the array elements need to be masked individually) as required by the VOTable standard. Bringing all of my hairy hacks over to the table class to make that work is the sticking point.

Does anyone know the status of this NEP? It seems like a much better way out of this mess:

https://github.com/numpy/numpy/blob/master/doc/neps/missing-data.rst

@mdboom - ok, so this ties back to #149. The NEP looks interesting, but I guess it'll be a while before we can make use of it? (not sure what the status is). Maybe we should implement our own masking scheme as part of #149? (feel free to comment on there if you have ideas). I guess the point you raise of masking individual elements in cells would pretty much rule out using numpy.ma for now.

I've attached a PR that is a reasonable first stab at this, completely throwing out masking for now. This might not be a bad first step to have something working, and then we can work on adding mask support to Table and then tie it in to this.

Any thoughts on this PR?

I don't know enough about VOTable to comment too intelligently, but it looks like a good start. The way I envisioned doing this for PyFITS, and thought you were going to do too, is actually create a subclass of astropy's Table and tack on format-specific methods/attributes/initializers/etc. For example I thought of making a FitsTable class and extending the existing Table class to support FITS columns.

That said, part of why I want to do that is that the existing FITS_rec class currently used in PyFITS to represent a table is clumsy and not very user-friendly, and I'm anxious to do away with it altogether. I don't know that the same could be said for votable.Table. So I can see why you might want an approach that doesn't change or remove the existing interface at all, but instead makes it possible to convert to an Astropy table. If nothing else it's a good transitional approach, since it doesn't break the existing interface.

But do you think in the long term you would want to consider something like I've proposed--a subclass of astropy's Table? Or would that not work very well in this case?

I had hoped to do it that way as well, but ultimately thought this was better. io.vo has always followed rather closely the XML structure of the VOTable format -- that is, if you're familiar with the XML format, you should already know the API, and I didn't want to lose that feature. The astropy.table class is quite different -- it stores its column metadata alongside the table, for example, whereas in VOTable, fields may be cross-referenced from other parts of the file, etc. I didn't want to lose the power of being able to set up the XML file in the way that makes the most sense for a particular application (and in some cases the way things are required by various VO protocols), so I think it's actually better to have a little "bump" that converts to/from a more generic table format.

The other problem is that the masking required by the VOTable standard doesn't match numpy.ma or what astropy.table does very well. Individual elements of array columns in a record array must be maskable. io.vo generates its own mask object in order to be able to do so. Of course, it doesn't work with numpy.ma, so it's not terribly easy to directly use it for operations, but with some elbow grease, the user can break the columns out into things that are numpy.ma compatible. There might be a way to make astropy.table sort of behave this way, but for the time being, all masking is thrown away when taking this approach. Since that breaks round-tripping, again, it's nice to have a "bump" and still be able to round-trip files perfectly if the user doesn't need the astropy.table interface.

Note also that a VOTable file is actually a heirachy of tables -- it's not a single table. PyFITS has this issue, too, of course, in that there is a list of header/data units, but in VOTable they are actually structured. That would be fine (we could still use a subclass of astropy.table for each of the actual tables within that), except the tables can cross-reference metadata from each other and there's just no good way to do that in astropy.table right now.

I should add, also, that I envision some sort of generic tool (a la ATpy) that would load tables in a generic way and always give you an astropy.table -- that would use this conversion function. That's what most people would use. The problem is that users that need to generate VOTables in a specific way could no longer do so if vo.tree.Table were a subclass of astropy.table. So best to keep both use cases supported.

Hmm... In principal I would have preferred it being a subclass of astropy.table.Table. Then I would have thought we would try to extend astropy.table.Table to support whatever is needed to have vo.table be an astropy.table.Table. That is, the way I imagined the Table class is that most table-like things would either subclass it or contain tables, rather than converter functions. But it's not obvious to me which is better, now that you mention the converter function style.

@astrofrog and @taldcroft - do you have any thoughts about this?

Oh, and I also your point that a VOTable is actually a heirarchy of tables is a good one... it's not obvious to me that we could ever support that in astropy.table.Table without it being very convoluted.

(Oh, @iguananaut - if you think you can make FITS binary table HDUs work as a subclass, that would be great - I guess in a way, that's more like what @mdboom is doing here, anyway)

As I said, I had hoped to subclass astropy.table.Table -- in practice that's just too limiting and wouldn't allow the full abilities of the VOTable to be exposed. Perhaps I should be more specific -- in VOTable, each column type is defined using a Field element. These Field elements do not need to be defined with a table, instead, each table may simply reference any number of Field elements elsewhere in the file. This is useful, for example, if the file is actually a number of tables all sharing the same format. It should remain possible to create such a file from astropy.io.vo. The Table class, or more specifically numpy arrays, do not directly support such a thing -- the dtypes of columns are copied into dtype describing all of the table's fields, so there is no way to maintain that link. I don't think it would even be desirable to do so. So I think that this approach really is the best one to maintain the two different use cases. Most users will just use some generic function "get me a table from whatever file I throw at you", and all of this will be hidden anyway.

PyFITS, of course, doesn't have these complications, so it may make more sense that it subclasses Table. I don't think the approach for one should have to dictate the approach for the other.

I also had in mind we would subclass from Table, but I can see why it is not ideal in this case, so I'm fine with this approach. I think the first priority is to have a conversion method like you have. This will already allow us to implement ATpy-like functionality in astropy.table (http://docs.astropy.org/en/latest/table/io.html), which is probably what we want most users to use as @mdboom said. With very little extra code, and this PR, it seems it will be easy to implement having:

from astropy.table import Table
t = Table('my_vo_table.xml')

Note that the identifier functions for Table could look at the first few bytes of the file to look for the VOTABLE string/identifier (rather than simplistically looking at extensions like ATpy does).

Note that the multiple table issue makes me think we might want some kind of TableCollection class (ATpy has TableSet) which also supports read/write methods. But I think that can wait.

@mdboom Thanks for the explanation: I figured there must be some features of the VOTable format that didn't translate over well. If the desire arises we can provide functions in the future that return a given file wrapped in a generic Table format a la ATPy like you mentioned.

In the meantime, yes, FITS tables translate over much more naturally. They still have quite a few oddities (variable length arrays especially) that complicate matters. But I don't think much if any functionality is lost by putting a more generic table format on top of it, so long as it's still possible to create a table from FITS column specifiers. Anyways, enough about pyfits for this thread.

Little late to the discussion but I think the current PR approach is sensible given the features of VOTable.

I guess what I was getting at before is that if we want to implement TableCollection or something like it, that would be a more natural superclass for VOTable, so an alternative approach would be to first go about implementing TableCollection with whatever features VOTable needs, and then making VOTable into a subclass of that.

So if we actually think we would want to do that, it might be unwise to do this now, because then there's two distinct ways of converting from VOTable to Table (the method added here, and whatever the TableCollection interface might add). And there should be one-- and preferably only one --obvious way to do it.

Having said that, at first glance I'm against a TableCollection class - just a list (or dict, or whatever) of Table objects seems like it would be better. It does then suffer from the problems VOTables rectify with the feature @mdboom mentioned (e.g., a single Field mapping to multiple columns), but at the astropy.table interface level, I think that might just add more confusion. So if we don't want those features outside the VOTable context, this PR is exactly what we want.

(Aside from this question, the PR looks mergeable to me, though).

@mdboom - note that the travis build is currently failing for this branch

I have rebased this on #429 (which makes the diff look enormous until that gets merged). Furthermore, votable now stores a single Numpy masked array for it's internal data, instead of a pair of (data, mask). Removed the warnings about losing masks when going to astropy.Table s, since this is no longer True.

Rebased on master so the diff is reasonable again.

This now appears to be failing only for numpu == 1.4.1 . Any idea what's going on there?

Numpy 1.4.1 doesn't have a dtype called np.bytes_, so in that case we have to use np.str_. Hopefully fixed now.

@mdboom - you commented that masks are no longer lost when going over to Table, which is not quite true yet, since the masking stuff isn't yet in the core repository. Should we reinstate that warning for now, until the masking is implemented in Table?

Also, once this is merged, I will submit a pull request linking vo into the Table class registry (http://docs.astropy.org/en/latest/table/io.html#creating-a-custom-reader-writer). This requires a little thought on treating files with multiple tables, but this is already something we've had to deal with in ATpy, so it shouldn't be a problem.

Apart from possibly adding the warning back, this looks good to me.

There's perhaps some argument here that we should hold off merging this until the mask support is in Table. I think the two should probably be tested together and any kinks at either end worked out.

This looks fine to me now... @mdboom, is there a reason not to merge this now and then do a separate PR for the mask support? I'm just thinking it might be easier for merging in case any other PR's for vo show up.

Probably no reason not to merge. I will add the warning about losing mask back in, as already pointed out, however.

I'd vote for merging this now and then we can always update things when merging in masking if there are any issues.

Ok. I've put the warning back in about the mask being thrown out.

BTW: The Travis build failure currently appears to be a false negative -- the build crashed on a couple of the configurations.

This looks good to me now.

Looks good to me, too - @mdboom I'll leave it to you to merge in case you had some last-minute changes in mind.