Refactor/rethink aspects of the Table I/O

@taldcroft

This is a top-level issue to keep track of refactoring/rethinking of the Table (and by extension NDData) I/O and to have a discussion about these things since several of them overlap.

At the moment, the Table class can be used to read/write to a number of formats, but there are several issues we need to address for 0.3:

Refactor Table readers and writers to use classes rather than free functions (Refactor Table readers and writers to use classes rather than free functions... #962)
Table.read and Table.write should provide useful docstrings (Table.read and Table.write should provide useful docstrings #1011). At the moment, this is one of the biggest annoyances of Table.read and Table.write for users, so I'm hoping we can somehow integrate this with the issue above (using classes) which could have some kind of docstring method that can be used to dynamically add information to the Table.read and Table.write docstrings. Similarly, we need to figure out where this should go in the docs.
@taldcroft suggested that the overwrite option for writers should only get rid of the old file at the last minute in case an exception is raised in the writer before the writing part.
We need to decide whether we want to make meta into a more complex object. An issue that was raised in table.write FITS writer? #591 is that currently the meta ordered dictionary cannot correctly store a FITS header, which requires support for duplicate keywords that have a certain order, e.g.:
```
CTYPE1 = "RA---TAN"
COMMENT some comment here
CTYPE2 = "DEC--TAN"
COMMENT another comment here
```
and if users read this in via Table.read and write out with Table.write (using the simple reader/writer in table.write FITS writer? #591) this ordering won't be preserved. Furthermore, keyword comments are currently dropped. So the question is, should tables from FITS files have a special meta object, for example a Header instance? How would that play with other formats when writing out in those formats? More generally, should we have the option to store format-specific constructs in Table? Or should we make it so that meta is more complex and can handle e.g. the peculiarities of FITS and other formats?

Now @iguananaut is also working on making FITS tables be Astropy Table objects by default, so then the question is, how does this fit in? Presumably the FITS tables will be a sub-class of the Table class, so should some of these format-specific issues go in there, and we just tell users that if they want round-tripping for a specific format, they can use that sub-class instead? Alternatively, Table.read could be a factory method that could return e.g. a FITSTable, etc.

So more generally, when do users use Table, and when do they use sub-classes?
Along these lines, we need to make it clearer to users when they should use Table, and when they should directly use e.g. io.ascii or votable for instance.

This is a bit of a brain dump, but I just wanted to start the conversation/thinking about this. For now, I'm just putting this here on GitHub since I think it's quite a technical discussion, but if we reach a point where we have fundamental disagreements, we can always take it to the list?

cc @taldcroft @mdboom @iguananaut @eteq

As discussed in #591, it would be good to have a Hangout to discuss these issues, but maybe people can already think about it and give their thoughts here first.

I agree with all of the points. The issue about overwrite is particularly important and not that hard to fix -- we should definitely be writing to a temporary and then mv it at the end if successful.

I also have, as a long-ish term goal, to make astropy.io.votable.Table instances subclasses of astropy.table.Table. (Note, I'm not saying astropy.io.votable.VOTable which represents an entire table file). What's there now was sort of the 20% effort for 80% of the benefit solution, but I still would like to get the 100% solution if possible. The end game, I believe, is that the user doesn't need to know when to use Table or a native object -- they would always be dealing with Table subclasses, but there would be escape hatches to get to the native object representation.

As for the meta issue -- maybe the thing to do is this: subclasses of Table are expected to provide two members: meta and a format-specific metadata member, so in the case of FITS, fits_meta. fits_meta would be the metadata in the native representation and interface, so in the case of FITS it would be io.fits.Header object. Then meta is required to be a flat dictionary-like view of that metadata. In the case of FITS, that could probably be the same object, because Header already duck-types as a dictionary. fits_meta, however, would support ordering of elements, comments etc., and meta would hide everything except some core metadata interface we define (which could be the Python dictionary interface plus whatever else we think is a useful common subset of the various metadata structures we support). So if the user cares about getting the FITS file perfect and wanted to write FITS-specific code, they would use fits_meta. If they only need an unordered key/value store, they would use meta. Different parts of user code may choose to use one or the other depending on context.

I'm suggesting this approach because (a) I don't think providing a superset interface for all of the different ways metadata is stored in various formats is feasible -- there is always going to be a need to escape down to the metal of a format to do something format-specific, and (b) there is probably a useful subset interface (either key/value pairs or key/value/comment triples) that is generic enough and useful enough that we should provide a common interface across formats for it.

Oh, regarding the docstring thing, a metaclass can handle that. For example:

>>> import random
>>> class MyThingMeta(type):
...     @property
...     def __doc__(cls):
...         return "Hello, I am your random docstring: %s" % random.random()
... 
>>> class MyThing(object):
...     __metaclass__ = MyThingMeta
... 
>>> help(MyThing)
Help on class MyThing in module __main__:

class MyThing(__builtin__.object)
 |  Hello, I am your random docstring: 0.805902368169
 |  
...
>>> help(MyThing)
Help on class MyThing in module __main__:

class MyThing(__builtin__.object)
 |  Hello, I am your random docstring: 0.381284667581
 |  
...

As to more substantive issues: I agree that at the high level users shouldn't have to think of what "table" type they use. If VOTable or FITS have their own Table subclasses, instances of those can be returned by Table.read() as appropriate. There's no reason a classmethod like Table.read can't return an instance of a subclass of Table.

I don't think it should return some kind of horrid Table wrapping a Table subclass or anything like that. FITSTables for example might have a few special FITS-specific features that a normal Table doesn't have (but it should always by duck-typeable as a normal Table--it is a subclass after all. Now when it comes to writing out to other formats the FITSTable class needs to be honest about what features it supports that may not be applicable to other formats (variable length arrays come to mind) have some way to converting that to a more general format (for something like VLAs there are several possible ways to do this; the user could possibly be given knobs for what approach to take).

As for FITS headers I don't think anything that fancy is really needed. The way the current Header option works is that each keyword is actually a (keyword, count) tuple where "count" refers to the nth card with that keyword. For most keywords count never goes above 0. For some like COMMENT it can be greater. I would suggest just using that scheme for dumping FITS headers into an OrderedDict. For keywords that only occur once just use the keyword string. For keywords that appear multiple times use tuples to distinguish each instance of that keyword.

@astrofrog

@astrofrog @iguananaut I really like the case-insensitive nature of fits headers so I'm all for not just using an OrderedDict as a meta data object. I also think that the comments (above and after the keywords) are very useful even for non-fits headers. I think a meta object as suggested by @astrofrog (that might also implement the meta data merging capabilities that @taldcroft put into table) would be a great enhancement to many areas of astropy.

(commenting for now on just the meta issue): I like the concept of an object for the metas that knows how to merge itself with other metas. Internally it could still store things as an OrderedDict when desired, and just implement a Mapping interface that accesses it. io.fits can make a subclass that's OrderedRepeatMeta or whatever, and it can know how to combine with similar metas, as well as "downgrade" in some sensible way to the base meta (which I would say should not support repeats. Although perhaps others think this is crucial.)

As a minor aside, I do not think we should adopt the case-insensitive nature of fits headers as standard - it leads to a fair amount of confusion because string comparisons are case-sensitive, and the fact that the header keys are not tricks you into thinking otherwise if you're not careful (leading to subtle bugs/frustration). If you really want it all to be the same case, there can always be a meta subclass (as that's a subset of functionality - just wrap everything in upper or lower as relevant), or stick to your own preferred convention when reading or writing FITS files.

I feel like this metadata thing is being overthought a little bit, but I don't have anything more constructive to add than that at the moment--I need to think about it a bit more myself (perhaps thusly contributing to my charge of "overthinking"). Also I agree, case insensitivity is Bad, and the fact that pyfits supports it is only because CFITSIO supports it, and it makes things harder not easier. It also makes a lot of code that uses it look sloppy--there's a tendency I've seen in a lot of astronomer code to arbitrarily mix the case used to access FITS keywords.

@taldcroft

@taldcroft @astrofrog This has an nddata label. I wonder if we should close this for now. Despite it being interesting nothing has happened for over a year (no criticism intended - just an observation).

Some of this is being done in #3090 - I would vote to keep this issue open for now, and when #3090 is finished/merged we can consider if any of the issues here are do-able or whether we should close it.

@astrofrog

@astrofrog @taldcroft - is this still on the table?

I'm in favor of closing this...

👍 on closing. In the 6 years hence there hasn't been any real movement forward and the existing structure has not been terribly problematic.

I agree, let’s close

Navigation Menu

Repository navigation

Refactor/rethink aspects of the Table I/O #1046

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Refactor/rethink aspects of the Table I/O #1046

Description

Activity

astrofrog commented on May 3, 2013

mdboom commented on May 3, 2013

embray commented on May 3, 2013

embray commented on May 3, 2013

wkerzendorf commented on May 3, 2013

eteq commented on May 4, 2013

embray commented on May 7, 2013

wkerzendorf commented on Jan 16, 2015

astrofrog commented on Jan 16, 2015

bsipocz commented on Jun 19, 2019

mwcraig commented on Sep 24, 2019

taldcroft commented on Sep 24, 2019

astrofrog commented on Sep 25, 2019

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions