Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Astropy ASCII tables #762

Merged
merged 4 commits into from
Oct 22, 2015
Merged

Conversation

astrofrog
Copy link
Member

This moves the Astropy table readers to a dedicated file. We now try and read in any file that cannot otherwise be read (priority=0) as a last attempt, which allows us to read a number of Astropy-friendly formats that could not be read before.

cc @teuben

@astrofrog
Copy link
Member Author

@ChrisBeaumont - this gets rid of the ascii.glue format. I found that by re-arranging how the ASCII readers are dealt with, we can read in files without any issues (basically we always try and read with Table.read(..., format='ascii') first, which avoids errors related to ambiguities. Do you see any reason (other than performance) why we shouldn't do what's done in this PR?

@ChrisBeaumont
Copy link
Member

The ascii.glue format was initially added to sidestep some of astropy's format guessing (eg astropy/astropy#1935 (comment)). For that specific comment at least the original issues were caught by Glue's test suite, so in principle change that satisfies our tests should be fine.

It's hard to know when it's safe to change our format guessing strategy, since we don't have test cases for all the various ways Table.read has broken on us in the past (many of which have been segfaults). But these changes at least look reasonable, so maybe we should try them out (and make sure to add new test cases as issues come up).

@astrofrog
Copy link
Member Author

@ChrisBeaumont - ok, thanks! If this passes, I'll go ahead and merge, and will try and include tests for more formats later (and every time users report an issue)

@astrofrog astrofrog added this to the 0.6 milestone Oct 22, 2015
astrofrog added a commit that referenced this pull request Oct 22, 2015
@astrofrog astrofrog merged commit 323898a into glue-viz:master Oct 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants