Fixes issue #3552: Mention Table interface in "Creating a New Table File" in FITS docs by vinayak-mehta · Pull Request #3558 · astropy/astropy

vinayak-mehta · 2015-03-01T12:14:44Z

embray · 2015-03-02T21:24:38Z

Thanks, this seems like a good start. Though I would probably write this differently, ultimately. I would want to move it closer to the top of the section and go through a more complete example (also with links to the Table docs). Perhaps showing how to create the same table using both the Table interface, and the old lower-level PyFITS interface, and also an explanation as to why the former should be preferred in most cases.

vinayak-mehta · 2015-03-03T03:43:02Z

@embray - Please point out the mistakes and help me refine it.

embray · 2015-03-03T18:21:51Z

@vortex-ape Thanks, this is looking good. I don't think there's much in the way of "mistakes" though I have a few other comments. I'll get back to it in a bit. But this is definitely on the right track for what I'm looking for.

embray · 2015-05-18T15:47:16Z

Thanks again for this, but I'm probably going to finish up this update to the docs myself, though I will include your example and make sure you're credited. I just want to go a little more in-depth, and actually refactor the docs altogether to emphasize this, and other simpler options for creating table HDUs. This is perfectly good work, but it's only a start.

hamogu · 2017-03-16T01:00:57Z

current fits maintaienrs @embray @MSeifert04 @pllim : I'm just looking through old, open PRs. Is there any reason this has not been merged yet? Yes, somebody could rework the docs more fully but that has not happened for the last two years, so I suggest merging this as is.
(The Travis failure seems unrelated, but in any case, somebody with the right priviliges should restart Travis, since that run was about 2 years ago.)

pllim

The PR is so old that simply restarted the failed test wouldn't work. It needs to be rebased (despite no merge conflict).

Would also be nice to post a screenshot of rendered HTML of this change.

@vinayak-mehta , sorry for the long delay. Erik is no longer involved and I wasn't signed up as a "deputy" then. If you cannot follow-up on this, just let me know and I'll take over. Since this PR existed before "allow edits from maintainers" was introduced, I will need to take over via a new PR but credit you for your original commits.

pllim · 2017-03-16T17:07:09Z

+.. note::
+
+    If you want to create a simple binary FITS table with no other HDUs,
+    you should use the Table_ interface in Astropy instead and then 


It is simpler to just do

:class:`~astropy.table.Table`

This way, "stable" will properly link to the API in "stable", "latest" to "latest", and so on.

pllim · 2017-03-16T17:08:04Z

+    write to FITS.
+
+    >>> from astropy.table import Table
+    >>> t = Table([[1,2],[4,5],[7,8]], names=('a','b','c'))


Wouldn't hurt to follow PEP8 here and add spaces after commas.

pllim · 2017-03-16T17:08:41Z

+
+    When using "lower-level" FITS interface, it is not usually necessary to
+    create column object explicitly as long as the data is stored in a 
+    `structured array <http://docs.scipy.org/doc/numpy/user/basics.rec.html>`_. 


https now.

pllim · 2017-03-16T17:09:05Z

+
+    - Code complexity is decreased.
+
+.. _Table: http://astropy.readthedocs.org/en/latest/api/astropy.table.Table.html#astropy.table.Table


This can be removed as per my suggestion above.

pllim · 2017-03-16T17:09:35Z

+
+    Benefits:
+
+    - Code complexity is decreased.


Is it really necessary to make a bulleted list out of a single item like this?

MSeifert04 · 2017-03-16T17:16:02Z

+    >>> t = Table([[1,2],[4,5],[7,8]], names=('a','b','c'))
+    >>> t.write('table.fits', format='fits')
+
+    Same code in PyFITS would look like this:


PyFITS? 😄

Now that you mention it, PyFITS's move to astropy.io.fits is so long ago now that we can probably replace PyFITS reference with astropy.io.fits. 🤔

no "probably" about it. Please change it to astropy.io.fits. PyFITS has been deprecated for 3 years now (early 2014).

And I vaguely remember previous PRs that got rid of similar mentions of pyfits both in code and docs.

saimn · 2017-03-16T22:59:23Z

+    >>> t = pyfits.BinTableHDU.from_columns([
+    ...     pyfits.Column(name='a', array=c1),
+    ...     pyfits.Column(name='b', array=c2),
+    ...     pyfits.Column(name='c', array=c3)])


This doesn't work, a format is required, something like fits.Column(name='a', array=[1,2], format='K').

pllim · 2017-03-17T12:57:06Z

@SaraOgaz , are you going to Katniss this one too? Or should I take over? 😂

SaOgaz · 2017-03-17T14:04:52Z

@pllim depends on how early you want it, heh. Do we know that @vinayak-mehta won't get to it? Are they still around?

pllim · 2017-03-17T14:08:28Z

I think code freeze for v2 is May-ish, so we have some time yet.

pllim · 2017-03-17T14:09:19Z

But if you want people to know about this in latest doc, then ASAP would be nice. Not sure if @vinayak-mehta is still interested. This PR is 2 years old. He might have moved on to something else.

SaOgaz · 2017-03-17T14:11:51Z

In that case, sure. I'll try and get to that today. Might need some guidance to make sure @vinayak-mehta still get's the credit.

pllim · 2017-03-17T14:17:13Z

Here it is, Katniss: http://stackoverflow.com/questions/3042437/change-commit-author-at-one-specific-commit

bsipocz · 2017-03-17T14:31:14Z

@pllim @SaraOgaz - I always feel weird about these history hacks and credit amendments. What about cherry picking the original commit from here on top of the current master, and do the fixes in a separate commits? That case the first commit will be credited to @vinayak-mehta and will have the original date, too without any hacking involved. Any additional fix will be credited to the person taking up the stab and finishing the issue.

pllim · 2017-03-17T14:36:32Z

@bsipocz , I never done that before but I like the suggestion. What are the incantations involved? Thanks!

saimn · 2017-03-17T14:49:50Z

You can get a branch from this PR and continue to work on it (possibly squashing the commits in one):

git fetch upstream pull/3558/head:pr-3558
git checkout pr-3558

bsipocz · 2017-03-17T15:01:58Z

@pllim - The other option, start with a new branch on top of the current master, and pick the 3 commits from here.
git cherry-pick 39bc597edf80d25b77578c8b079b2c55d739d9e3 20b3dc05152b333650b67643a5f8d08f82a53582 335c72ccaae180b661b7cc5c34370c2e0755a72e

I agree with @saimn, that it would be then best to squash them together.

bsipocz · 2017-03-17T15:05:13Z

hmm, in my solution you may need to add the fork of @vinayak-mehta as a remote to be able to see these commits (I usually have the forks of many people as remotes, so haven't tried this without it).

pllim · 2017-03-21T15:32:34Z

I took over and moved it to #5893 . This will close when that one is merged. FYI.

hamogu · 2017-03-22T17:50:38Z

Thanks to everybody who contributed work on this PR and the discussion!

You might hear from me again: I've decided to comb through all open PRs to find those that are still open while "almost but not quite finished". Some are really good ideas, but the longer we wait, the more extra work somebody has to put in to finish them (rebase etc.). So, I'll try to call out those that seem easy to finish (or do it myself if I know enough about the relevant sub-package). Also, the number of open PRs can be intimidating to newcomers.

vinayak-mehta · 2017-03-23T08:30:47Z

Sorry for not being able to address this, saw it just today.

pllim · 2017-03-23T13:59:45Z

@vinayak-mehta , no worries!

Mentioned Table interface in FITS docs

39bc597

vinayak-mehta mentioned this pull request Mar 1, 2015

Mention Table interface in "Creating a New Table File" in FITS docs #3552

Closed

Updated documentation

20b3dc0

updated documentation

335c72c

astrofrog assigned embray Mar 21, 2015

embray force-pushed the master branch from 63540e0 to 34f4c29 Compare October 5, 2015 21:18

mwcraig added the Docs label Feb 25, 2016

pllim added io.fits no-changelog-entry-needed table labels Mar 16, 2017

pllim unassigned embray Mar 16, 2017

pllim added this to the v2.0.0 milestone Mar 16, 2017

pllim requested changes Mar 16, 2017

View reviewed changes

MSeifert04 reviewed Mar 16, 2017

View reviewed changes

saimn reviewed Mar 16, 2017

View reviewed changes

pllim mentioned this pull request Mar 20, 2017

Split pytest plugins out into individual modules #5770

Closed

pllim added a commit to pllim/astropy that referenced this pull request Mar 21, 2017

Applied comments from astropy#3558 with extra clean-up.

16da403

pllim mentioned this pull request Mar 21, 2017

Mention Table interface in "Creating a New Table File" in FITS docs #5893

Merged

pllim added the Duplicate label Mar 21, 2017

pllim removed this from the v2.0.0 milestone Mar 21, 2017

pllim removed the table label Mar 21, 2017

pllim closed this in #5893 Mar 22, 2017


		- Code complexity is decreased.

		.. _Table: http://astropy.readthedocs.org/en/latest/api/astropy.table.Table.html#astropy.table.Table

Conversation

vinayak-mehta commented Mar 1, 2015

Uh oh!

embray commented Mar 2, 2015

Uh oh!

vinayak-mehta commented Mar 3, 2015

Uh oh!

embray commented Mar 3, 2015

Uh oh!

embray commented May 18, 2015

Uh oh!

hamogu commented Mar 16, 2017

Uh oh!

pllim 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!

pllim Mar 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

pllim commented Mar 17, 2017

Uh oh!

SaOgaz commented Mar 17, 2017

Uh oh!

pllim commented Mar 17, 2017

Uh oh!

pllim commented Mar 17, 2017

Uh oh!

SaOgaz commented Mar 17, 2017

Uh oh!

pllim commented Mar 17, 2017

Uh oh!

bsipocz commented Mar 17, 2017

Uh oh!

pllim commented Mar 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saimn commented Mar 17, 2017

Uh oh!

bsipocz commented Mar 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bsipocz commented Mar 17, 2017

Uh oh!

pllim commented Mar 21, 2017

Uh oh!

hamogu commented Mar 22, 2017

Uh oh!

vinayak-mehta commented Mar 23, 2017

Uh oh!

pllim commented Mar 23, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

pllim Mar 16, 2017 •

edited

Loading

pllim commented Mar 17, 2017 •

edited

Loading

bsipocz commented Mar 17, 2017 •

edited

Loading