Better docs and infrastructure for unified read/write functions by taldcroft · Pull Request #8255 · astropy/astropy

taldcroft · 2018-12-10T00:16:32Z

This PR is a decent-sized update to the unified I/O infrastructure, with the main driver being to make it possible to get format-specific documentation in a simple way. This is done with (e.g.):

>>> Table.read.help('ascii.latex')
>>> Table.write.help('fits')
>>> CCDData.read.help('fits')
>>> Table.read.help()  # Generic reader help
>>> Table.read?  # Basically the same thing

This prints output to the console via a pager (pydoc.pager) and includes the generic read/write interface help and format-specific docs.

In order to make the read.help and write.help methods in a clean way I ended up with some new general-purpose infrastructure for defining read and write methods as class descriptors. Along the way I cleaned up the io.ascii docs to make them more consistent within this framework.

I also did cleanup of the Table registry by removing the custom-installed 'ascii' and 'csv' entry points in favor of generic mechanisms now in place.

NOTE for future readers: the comments up through #8255 (comment) are mostly overtaken by the new code.

taldcroft · 2018-12-10T00:18:14Z


    return dat

+read.__doc__ = core.READ_DOCSTRING


Needed to avoid circular import.

mhvk · 2018-12-10T18:40:58Z

Yes to having this information!

A possible alternative implementation might be to have read have attributes which have the relevant docstring (and possibly can be used as short-cuts to reading), i.e.,

Table.read?
# Tells "look at table.read.format"
Table.read.fits?
# Gives information on how to read a latex-formatted file.

# MAYBE???
t = Table.read.fits('event.fits')  # same as Table.read('event.fits', format='fits')

To implement this, read will have to become an instance of a "TableReader" class that has the appropriate general docstring, a __call__ method that implements the current read class method, and a __getattr__ which helps get the format documenation. (Possibly, this could be pushed up to the io_registry.read implementation; not sure).

One advantage would be that it is more discoverable. Arguably, it is also a bit more pythonic, but it may well be too tricky to get right... Certainly your implementation here has the benefit of simplicity!

taldcroft · 2018-12-10T19:03:13Z

@mhvk - very nice idea, I like it on every level! The main question is whether the implementation can be kept manageable. Mostly I think it isn't too bad, but there is the issue of the hierarchy of readers, namely:

t = Table.read('file.fits')  # auto-identify
t = Table.read.fits('file.fits')
t = Table.read.ascii('file.dat')
t = Table.read.ascii.ecsv('file.ecsv')

So presumably a UnifiedReader class needs to have a __call__ method that calls a particular reader and an optional list of registered sub-readers (also UnifiedReader objects). (This should be generalized to any class that is supported in the unified I/O framework).

At that point one starts to think about solving the whole issue where the table __init__.py needs to import (in advance) every class that has a reader. I'm not sure at this point if the import time issue is a real driver.

mhvk · 2018-12-10T19:22:17Z

Yes, indeed, I do worry about the implementation. Though I don't think there is a need to "pre-import" too much - that can be done in the __getattr__ method.

The recursion may be trickier, as one wants to be sure not to reimplement things that python and/or the io registry already do - indeed, this is partially why I wondered if we could push part of this up to io.registry (i.e., should it be the module gaining the new Reader class, with Table.read just a thin veneer?)

astrofrog

I don't think using attribute notation would necessarily work since some formats (defined outside astropy) may not have Python-variable-compliant names?

In any case, this here is much better than the current situation, so I would actually propose that we consider merging this and then consider making more substantial changes if we really want.

This should be mentioned in the unified I/O docs though (and anywhere Table.read is mentioned and in the Table.read docstring)

astrofrog · 2018-12-10T23:03:00Z

+        if format and not args and not kwargs:
+            reader = io_registry.get_reader(format, cls)
+            reader_doc = ('Table.read() general documentation\n'
+                          '==================================\n')


The headings don't look quite right - the top heading seems a bit 'stuck' to the Table.read call - maybe try with heading lines above too? (since I guess we don't need to be ReST compliant here?)

taldcroft · 2018-12-11T10:06:20Z

Good points @astrofrog.

I suspect in the wild there are not many (probably zero) user-defined formats that have a name that doesn't work as an attribute. The most likely problem is - which is commonly translated to _.

But the issue of just getting something that works without going crazy (given the reality of time constraints) is well-taken. I can clean this implementation up a bit, include the writers, and we'll see where it goes.

astrofrog

@taldcroft - this looks great! In addition to the in-line comments, one thing I was wondering was whether when a user does:

Table.read.help('ascii')

it would make sense to just show the format-specific docs, but to include a sentence saying that for general information about the read method, just to do Table.read.help()? As a user, I feel like the workflow I might follow is to first check the help for Table.read, then to find out that I can do Table.read.help('ascii') so I don't necessarily want to see the general docs again. Alternatively, you could swap the order to have the format-specific docs first?

astrofrog · 2019-04-08T19:47:43Z

-    see: https://github.com/astropy/astropy-APEs/blob/master/APE6.rst.
+    """ECSV (Enhanced Character Separated Values) format table.
+
+    Th ECSV format allows for specification of key table and column meta-data, in


astrofrog · 2019-04-08T19:48:01Z

+        This parameter is ignored when the ``output`` arg is not a string
+        (e.g., a file object).
+    Writer : ``Writer``
+        Writer class (DEPRECATED).


I wonder if it's time to finally remove Reader/Writer?

Punting to #8567 and 4.0 release.

astrofrog · 2019-04-08T19:49:59Z

+    output : str, file_like
+        Output [filename, file-like object]. Defaults to``sys.stdout``.
+    format : str
+        Output table format. Defaults to 'basic'.


I wonder whether when this docstring is used for a specific format it would make sense to not list format? In other words, maybe that is a keyword that is specific to the generic read and write methods?

Good point but I can't think of a clean and easy way to handle this. For now I'd just say to live with this.

astrofrog · 2019-04-08T19:55:07Z

+    Get help on the writer for a particular format (e.g. 'fits') using the
+    ``help()`` method::
+
+      >>> CCDData.write.help('fits')  # Get detailed help on FITS writer


I wonder if we should also provide a way and document how to get a list of available format

astrofrog · 2019-04-08T19:56:30Z

 details on the implementation see :ref:`io_registry`.

+Getting started with Image I/O
+==============================


What is the rationale for having this section here? Should it not be in the CCDData docs?

The rationale was that the Unified I/O was never supposed to be just for Table, so I'm trying to make the docs more inclusive / symmetric with Table.

astrofrog · 2019-04-08T20:01:18Z

+                      .format(cls.__name__, method_name))
+            reader_doc = re.sub('.', '=', header)
+            reader_doc += header
+            reader_doc += re.sub('.', '=', header)


Can you add a newline after the heading? That is,

================================ Table.read general documentation ================================ Read and parse a data table and return as a Table.

instead of

================================ Table.read general documentation ================================ Read and parse a data table and return as a Table.

astrofrog · 2019-04-08T20:02:47Z

+
+    Notes
+    -----
+


Maybe remove the newline here? At the moment this has too many newlines in some cases:

Notes ----- The available built-in formats are:

taldcroft · 2019-04-11T11:18:38Z

@astrofrog - I think I have addressed all your comments.

bsipocz

My main comment is that using intersphinx linking rather than hard wired urls is preferable.
The other comment is whether it's possible to change the order of the doc, so the format specific part comes up before the generic one.

The rest is then just nitpicking.

bsipocz · 2019-04-11T21:05:55Z

+    ----------
+    table : str, file-like, list, pathlib.Path object
+        Input table as a file name, file-like object, list of strings,
+        single newline-separated string or pathlib.Path object .


could you link `pathlib.Path`

bsipocz · 2019-04-11T21:06:32Z

+
+    Reader : `~astropy.io.ascii.BaseReader`
+        Reader class (DEPRECATED)
+    encoding: str


Suggested change

encoding: str

encoding : str

bsipocz · 2019-04-11T22:11:33Z

+            table for each chunk.  The default is to return a single stacked table
+            for all the chunks.
+
+    Reader : `~astropy.io.ascii.BaseReader`


I think we normally remove the deprecated parameters from the docstring when they get deprecated. Same for Writer below.

bsipocz · 2019-04-11T22:12:34Z

+
+    See also:
+
+    - http://docs.astropy.org/en/stable/io/ascii/


same here, if possible use intersphinx instead

taldcroft · 2019-04-12T11:06:36Z

@bsipocz - comments all addressed, I think.

codecov · 2019-04-12T13:01:46Z

Codecov Report

❗ No coverage uploaded for pull request base (master@780d1f4). Click here to learn what that means.
The diff coverage is 88%.

@@            Coverage Diff            @@
##             master    #8255   +/-   ##
=========================================
  Coverage          ?   86.89%           
=========================================
  Files             ?      399           
  Lines             ?    59197           
  Branches          ?     1100           
=========================================
  Hits              ?    51442           
  Misses            ?     7114           
  Partials          ?      641

Impacted Files	Coverage Δ
astropy/io/ascii/daophot.py	`98.76% <ø> (ø)`
astropy/io/ascii/cds.py	`96.63% <ø> (ø)`
astropy/nddata/ccddata.py	`94.59% <ø> (ø)`
astropy/io/ascii/rst.py	`100% <ø> (ø)`
astropy/io/ascii/ipac.py	`97.47% <ø> (ø)`
astropy/io/ascii/html.py	`97.87% <ø> (ø)`
astropy/io/ascii/sextractor.py	`98.48% <ø> (ø)`
astropy/io/ascii/ecsv.py	`93.75% <ø> (ø)`
astropy/io/ascii/latex.py	`97.88% <ø> (ø)`
astropy/io/ascii/fixedwidth.py	`96.75% <ø> (ø)`
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 780d1f4...ebddb39. Read the comment docs.

bsipocz · 2019-04-12T19:21:33Z

Thanks @taldcroft!

pllim · 2019-04-19T20:32:18Z

Please rebase to resolve conflicts.

bsipocz · 2019-04-19T22:31:19Z

Rebased, this should be now good to go.

taldcroft added table unified-io Work in progress labels Dec 10, 2018

taldcroft commented Dec 10, 2018

View reviewed changes

astrofrog reviewed Dec 10, 2018

View reviewed changes

taldcroft mentioned this pull request Dec 11, 2018

Implement Table reader and writer for ASDF #8261

Merged

taldcroft changed the title ~~POC: better docs for unified read/write functions [skip-ci]~~ Better docs and infrastructure for unified read/write functions Dec 16, 2018

taldcroft added the Ready-for-final-review label Dec 16, 2018

taldcroft mentioned this pull request Jan 26, 2019

Add pandas backend support for Table read/write #8381

Merged

3 tasks done

pllim added this to the v3.2 milestone Feb 8, 2019

astrofrog self-requested a review February 26, 2019 16:18

taldcroft mentioned this pull request Apr 8, 2019

Added new astropy.timeseries sub-package #8540

Merged

astrofrog requested changes Apr 8, 2019

View reviewed changes

taldcroft force-pushed the unified-docs branch from e238ce9 to 94dc13c Compare April 11, 2019 11:05

taldcroft removed the Work in progress label Apr 11, 2019

taldcroft force-pushed the unified-docs branch from 94dc13c to d4c0f44 Compare April 11, 2019 11:11

astrofrog self-requested a review April 11, 2019 11:57

bsipocz reviewed Apr 11, 2019

View reviewed changes

taldcroft force-pushed the unified-docs branch from d947005 to 8c0ce56 Compare April 12, 2019 12:34

bsipocz approved these changes Apr 12, 2019

View reviewed changes

taldcroft mentioned this pull request Apr 13, 2019

Consider a generic mechanism for inheriting keywords from baseclass methods #8578

Open

taldcroft added 21 commits April 19, 2019 15:23

Add table connect.py module [skip ci]

1c3b226

More progress including handling write correctly

ab9ca01

Use aliasing not custom read funcs for ascii, csv formats

74d23be

Make write actually work again

a174378

Update io.ascii class docstrings for consistent style

0cf34cc

Add back io.ascii.ui.write docstring

81c56f0

Add implementation docs, error handling, output option

e4cf64a

Add tests

f18bd33

Skip TableRead/Write doctests

f63bffe

Generalize read/write method to make this applicable beyond Table

40b4159

Apply the new unified I/O scheme to CCDData

82be42f

Make format be optional in help()

e1ad9c7

More docs, in particular describing use of help()

82b91a1

Fix problem in doctest skipping

ad1075a

Fix a subtle test collection problem in testing

f816fe6

Include example in docstrings of help() to list formats

ae4a00f

Improve output formatting

91b8994

Add a new method to list available I/O formats

b656d4d

Update change log

2989ec7

Skip help docs test in nddata/index.rst

84d571e

Only generic or specific docs, not both; plus various fixes from review

ebddb39

bsipocz force-pushed the unified-docs branch from 8c0ce56 to ebddb39 Compare April 19, 2019 22:31

pllim merged commit ed4ccb4 into astropy:master Apr 20, 2019

taldcroft added the whatsnew-needed label May 10, 2019

This was referenced Jun 18, 2019

Table.read and Table.write should provide useful docstrings #1011

Closed

Unified file read/write interface: binary votable format #7352

Closed

taldcroft deleted the unified-docs branch October 1, 2019 20:45

taldcroft mentioned this pull request Feb 13, 2021

Remove orphaned TableReadWrite class #11324

Merged

pllim mentioned this pull request Jul 6, 2023

Importing astropy.coordinates fails with python -OO flag #15028

Closed


		See also:

		- http://docs.astropy.org/en/stable/io/ascii/

Conversation

taldcroft commented Dec 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented Dec 10, 2018

Uh oh!

taldcroft commented Dec 10, 2018

Uh oh!

mhvk commented Dec 10, 2018

Uh oh!

astrofrog left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taldcroft commented Dec 11, 2018

Uh oh!

astrofrog left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taldcroft commented Apr 11, 2019

Uh oh!

bsipocz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

taldcroft commented Apr 12, 2019

Uh oh!

codecov Bot commented Apr 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

taldcroft commented Dec 10, 2018 •

edited

Loading

codecov Bot commented Apr 12, 2019 •

edited

Loading