Skip to content

Fix mistake in ECSV reader docstring and make example testable - #5450

Merged
taldcroft merged 2 commits into
astropy:masterfrom
cdeil:issue-5450
Nov 6, 2016
Merged

taldcroft merged 2 commits into
astropy:masterfrom
cdeil:issue-5450

Conversation

@cdeil

@cdeil cdeil commented Nov 5, 2016

Copy link
Copy Markdown
Member

@taldcroft - Is the basic ECSV example from the Astropy docs broken again?

http://docs.astropy.org/en/latest/api/astropy.io.ascii.Ecsv.html

I thought we fixed it and I checked that it worked in #5263, but now I get an error again:

from astropy.table import Table
ecsv_content = """# %ECSV 0.9
# ---
# datatype:
# - {name: a, unit: m / s, type: int64, format: '%03d'}
# - {name: b, unit: km, datatype: int64, description: This is column b}
a b
001 2
004 3
"""
Table.read(ecsv_content, format='ascii.ecsv')

gives this error:

KeyError                                  Traceback (most recent call last)
<ipython-input-11-8cfe8b05e3da> in <module>()
      9 004 3
     10 """
---> 11 Table.read(ecsv_content, format='ascii.ecsv')

/Users/deil/Library/Python/3.5/lib/python/site-packages/astropy-1.3.dev16202-py3.5-macosx-10.12-x86_64.egg/astropy/table/table.py in read(cls, *args, **kwargs)
   2333         passed through to the underlying data reader (e.g. `~astropy.io.ascii.read`).
   2334         """
-> 2335         return io_registry.read(cls, *args, **kwargs)
   2336 
   2337     def write(self, *args, **kwargs):

/Users/deil/Library/Python/3.5/lib/python/site-packages/astropy-1.3.dev16202-py3.5-macosx-10.12-x86_64.egg/astropy/io/registry.py in read(cls, *args, **kwargs)
    466 
    467         reader = get_reader(format, cls)
--> 468         data = reader(*args, **kwargs)
    469 
    470         if not isinstance(data, cls):

/Users/deil/Library/Python/3.5/lib/python/site-packages/astropy-1.3.dev16202-py3.5-macosx-10.12-x86_64.egg/astropy/io/ascii/connect.py in io_read(format, filename, **kwargs)
     35     from .ui import read
     36     format = re.sub(r'^ascii\.', '', format)
---> 37     return read(filename, format=format, **kwargs)
     38 
     39 

/Users/deil/Library/Python/3.5/lib/python/site-packages/astropy-1.3.dev16202-py3.5-macosx-10.12-x86_64.egg/astropy/io/ascii/ui.py in read(table, guess, **kwargs)
    339                                              ' with fast (no guessing)'})
    340         else:
--> 341             dat = reader.read(table)
    342             _read_trace.append({'kwargs': new_kwargs,
    343                                 'status': 'Success with specified Reader class '

/Users/deil/Library/Python/3.5/lib/python/site-packages/astropy-1.3.dev16202-py3.5-macosx-10.12-x86_64.egg/astropy/io/ascii/core.py in read(self, table)
   1151 
   1152         # Get the table column definitions
-> 1153         self.header.get_cols(self.lines)
   1154 
   1155         # Make sure columns are valid

/Users/deil/Library/Python/3.5/lib/python/site-packages/astropy-1.3.dev16202-py3.5-macosx-10.12-x86_64.egg/astropy/io/ascii/ecsv.py in get_cols(self, lines)
    143                 if attr in header_cols[col.name]:
    144                     setattr(col, attr, header_cols[col.name][attr])
--> 145             col.dtype = header_cols[col.name]['datatype']
    146             # ECSV "string" means numpy dtype.kind == 'U' AKA str in Python 3
    147             if not six.PY2 and col.dtype == 'string':

KeyError: 'datatype'

I think the problem now is that the column info has the key type instead of datatype?

# - {name: a, unit: m / s, type: int64, format: '%03d'}

Was there a change in the ECSV reader or was this example always broken?

It looks like this issue is also present in the original APE, which I guess is still the reference to look at for ECSV
https://github.com/astropy/astropy-APEs/blame/master/APE6.rst#L319

@pllim pllim added table io.ascii Affects-dev PRs and issues that do not impact an existing Astropy release labels Nov 2, 2016
@taldcroft

Copy link
Copy Markdown
Member

Looks like I merged #5263 prematurely since @mhvk had noted that your fix wasn't actually enough to make it work. The docstring example was always broken, while the example shown in the APE6 spec is correct and the keyword should be datatype not type.

@cdeil

cdeil commented Nov 5, 2016

Copy link
Copy Markdown
Member Author

There was one more case where an example in APE6 incorrectly had "type:" instead of "datatype:".
Fixed in astropy/astropy-APEs#16

@cdeil

cdeil commented Nov 5, 2016

Copy link
Copy Markdown
Member Author

I've attached a commit that applies the "type:" -> "datatype:" fix, and changes the example so that it's run by doctest.

@taldcroft taldcroft self-assigned this Nov 6, 2016
@taldcroft

Copy link
Copy Markdown
Member

@cdeil - thanks, looks good to me for merge. I'm just not sure if a doc fix like this should be back-ported to LTS. @astrofrog or @eteq?

@astrofrog

Copy link
Copy Markdown
Member

@taldcroft - I would say yes, just in case. It's not much work to backport simple patches like this, so no harm.

@taldcroft taldcroft added this to the v1.0.11 milestone Nov 6, 2016
@taldcroft
taldcroft merged commit 98f173b into astropy:master Nov 6, 2016
@taldcroft taldcroft changed the title ECSV column 'type' or 'datatype'? Fix mistake in ECSV reader docstring and make example testable Nov 6, 2016
eteq pushed a commit that referenced this pull request Dec 21, 2016
Fix mistake in ECSV reader docstring and make example testable
eteq pushed a commit that referenced this pull request Dec 21, 2016
Fix mistake in ECSV reader docstring and make example testable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Affects-dev PRs and issues that do not impact an existing Astropy release io.ascii no-changelog-entry-needed Ready-for-final-review table

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants