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

Docs io unified reader writer table #5112

Merged

Conversation

jpowerwa
Copy link

Documentation update consolidating information regarding ascii formats supported by unified file read/write access. In response to #5046.

@mwcraig
Copy link
Member

mwcraig commented Jun 17, 2016

@jpowerwa -- thanks for the contribution! Just wanted to give you a heads up that it may be a few days until this is reviewed. A release of v1.2 is almost ready, and attention will be focused there for a bit.

@mwcraig mwcraig added this to the v1.2.1 milestone Jun 17, 2016
@taldcroft
Copy link
Member

@jpowerwa - thanks, combining into one table would be good. We don't want to lose the existing information about Auto-identify and Deprecated, so you should put those columns back in. Also, the non-ASCII or ASCII-shortcut readers like fits and aastex (respectively) should be still included.

A good follow-on idea would be to actually remove the deprecated readers in astropy 1.3. They've been marked deprecated for quite some time now.

@jpowerwa
Copy link
Author

@taldcroft - thanks for the feedback.

The original issue suggested removing the deprecated rows from the table, so I went ahead and did it, but adding them back is simple enough. My understanding is that these are all ASCII formats, and that it is not the format but just the naming convention that is deprecated in favor of the 'ascii.*' naming convention. Is that correct?

I made the other changes to handle a subtle issue that I encountered when combining the tables, and I'd appreciate your opinion on a better way to handle that. The issue is that the inclusion of the binary formats in the same table as the ASCII formats means that the table needs to live outside of the ASCII formats section even though most of the information in the table is only pertinent for the ASCII formats. Also for the ASCII formats, the "Suffix" column is representative and more specific than the "Auto-identify" column. I suspect this was the reason for the redundant tables in the first place.

My hope was that it would be okay to drop the binary formats from the table, since each binary format has its own section in the doc, since all the binary formats are readable and writable, and since auto-identification is supported for all the binary formats, but by a different mechanism that for the ASCII formats, as auto-identification is based on file data for the binary formats and file suffix for the ASCII formats.

Dropping the binary format rows made it possible to move the table into the ASCII formats section and to replace the "Auto-identify" column with the "Suffix" column, which seemed like an improvement. I added the paragraph naming and linking each of the binary format at the "Built-in table readers/writers" level as an attempt to surface those formats at the top-level, as they had been when the table was at that level.

If you feel that pulling the binary formats from the table makes them less discoverable, one possibility would be to reorganize the structure of the "Built-in table readers/writers" section to have two parallel sub-sections: "ASCII formats" and "Binary formats". The "ASCII formats" section would be as it is now, and the "Binary formats" section could explain that auto-detection for those formats works by examining the file data and then contain the subsections for the specific formats.

I'd love to hear your thoughts.

@taldcroft
Copy link
Member

@jpowerwa -

  • I guess it is time to remove the deprecated formats (at least from the docs table), so 👍 on that.
  • Good point that auto-identify and suffix are basically redundant, so just having the suffix column is fine.
  • Distinguishing binary vs ASCII is not that clear (e.g. is votable binary or ASCII? The content is actually all ASCII, except in the odd cases where it isn't. Same with a FITS ASCII table).
  • I definitely want to see all available formats shown in a single summary table.

So I think there is a clear path to a single succinct table at the top.

@jpowerwa
Copy link
Author

@taldcroft - Thanks for the guidance. I will do as you recommend.

@astrofrog astrofrog modified the milestones: v1.2.1, v1.2.2 Jun 24, 2016
Promote single table to top-level of built-in readers/writers section

as per reviewer feedback
@jpowerwa
Copy link
Author

@taldcroft - I have updated the pull request according to your feedback. I picked what seemed like reasonable "Description" links for fits, hdf5 and votable. Happy to update those if you would rather see those go elsewhere.

@astrofrog
Copy link
Member

@taldcroft - does this look good to you?

.. _H5PY: http://www.h5py.org/

Reading/writing from/to HDF5_ files is
supported with ``format='hdf5'`` (this requires H5PY_
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The h5py link should be lowercase.

@taldcroft
Copy link
Member

@jpowerwa - sorry for the long delay. Looks good now apart from the minor comment about the h5py link.

@jpowerwa
Copy link
Author

@taldcroft: Updated H5PY -> h5py as requested.

@eteq eteq modified the milestones: v1.3.1, v1.2.2 Jan 4, 2017
@eteq
Copy link
Member

eteq commented Jan 4, 2017

Oops, looks like this never got in for 1.2.2 ... But I've re-milestoned it for 1.3.1 . @taldcroft, does this look good to you, aside from the docs build failure (which I think is a red herring, so I restarted it now)?

Copy link
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@taldcroft taldcroft merged commit 430bc71 into astropy:master Jan 6, 2017
@astrobot
Copy link

astrobot commented Jan 6, 2017

@taldcroft - thanks for merging this! However, I noticed the following issue with this pull request:

  • Changelog entry not present (or pull request number missing) and neither the Affects-dev nor the no-changelog-entry-needed label are set

Would it be possible to fix this? Thanks!

If you believe the above to be incorrect (which I - @astrobot - very much doubt) you can ping @astrofrog

@taldcroft
Copy link
Member

@jpowerwa - thanks! Sorry again for the slow turnaround.

bsipocz pushed a commit that referenced this pull request Feb 14, 2017
…table

Improve unified I/O docs reader/writer table
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.

None yet

6 participants