Skip to content

Add high-level docs - #42

Merged
cdeil merged 25 commits into
astropy:masterfrom
jberlanga:docs_sphinxext
Aug 28, 2015
Merged

cdeil merged 25 commits into
astropy:masterfrom
jberlanga:docs_sphinxext

Conversation

@jberlanga

Copy link
Copy Markdown
Member

Added additional docs/*.rst files for documentation.

Addresses Issue #41.

@eteq eteq mentioned this pull request Jul 28, 2015
@cdeil cdeil added this to the 0.1 release milestone Jul 28, 2015
@cdeil cdeil self-assigned this Jul 28, 2015
@cdeil cdeil changed the title High-level docs. Add high-level docs Jul 28, 2015
@cdeil

cdeil commented Jul 28, 2015

Copy link
Copy Markdown
Member

I've put myself as assignee on this one, i.e. I'll review and eventually merge it.
@jberlanga – Should I have a look now or later?

The travis-ci build currently fails because of this Sphinx warning: https://travis-ci.org/astroplanners/astroplan/jobs/72916157#L904

@jberlanga

Copy link
Copy Markdown
Member Author

@cdeil Later would be better--I'm traveling tomorrow and then moving apartments, so the docs may still be pretty meager until early next week.

I'm pretty sure I know how to fix the Travis CI build error.

@jberlanga

Copy link
Copy Markdown
Member Author

@astroplanners/astroplanners-contributors

You can find docs in progress at: jberlanga.github.io/astroplan

@bmorris3

Copy link
Copy Markdown
Contributor

❗ The tutorials in progress look awesome. I don't use packages if they don't have docs like these, so I'm really glad to see them coming along!

@ejeschke

Copy link
Copy Markdown
Contributor

I agree, really looks good!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would show an example where astroplan does the time conversion for you:

from datetime import datetime
time = subaru.datetime_to_astropy_time(datetime(2015, 06, 16, 2, 0, 0))

@cdeil cdeil mentioned this pull request Jul 30, 2015
Comment thread docs/index.rst Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You could instead make this a warning to the user that the API could change because this is still very much in active development (re-phrase it in your own words). You can make a warning by doing:

.. warning::

    Text for the warning

and sphinx will render it nicely.

@adrn

adrn commented Aug 3, 2015

Copy link
Copy Markdown
Member

@jberlanga Do you want us to do a line-by-line review of this PR, or would you like some more time to refine things?

@jberlanga

Copy link
Copy Markdown
Member Author

@adrn & @astroplanners/astroplanners-contributors : I spent most of last week traveling/moving, so I'm a bit behind on this PR--best to wait until I push some more content--I'll alert you.

@jberlanga

Copy link
Copy Markdown
Member Author

Hi @astroplanners/astroplanners-contributors! I've made a substantial update to this branch--check out http://jberlanga.github.io/astroplan/ to see a preview of the docs.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I haven't thought about it much, but I think I'm -1 on using the ipython Sphinx directive.

As far as I can see, the main difference wrt. the normal Python Sphinx directive is the input / output cell numbers.
And those don't carry useful info, i.e. are more distracting than helpful in my opinion.

So I'd suggest re-writing the examples that show input and output using the Python prompt >>> prefix for inputs:

.. code-block:: python
    >>> subaru.parallactic_angle(time, altair)
    −0.640582rad
    >>> subaru.parallactic_angle(time, vega)
    −0.465298rad

If it's about getting this format quickly for the docs, what I sometimes do is call %history in IPython, and then copy & paste the relevant commands into a new Python shell, then copy & paste & indent by 4 spaces that to the RST file.

If you agree to this change, the point is irrelevant. But in case not, the IPython directive is currently not found in the Sphinx build, resulting in this error:
https://travis-ci.org/astroplanners/astroplan/jobs/74846421#L874

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fair point on in/out #'s being distracting. It should be easy enough to replace these bits.

@cdeil

cdeil commented Aug 10, 2015

Copy link
Copy Markdown
Member

@jberlanga – Thanks for the update!

Before you start filling in lots of examples, @astroplanners/astroplanners-contributors is such a "HOWTO" or "Cookbook" the best way? (see http://jberlanga.github.io/astroplan/howtoget.html)
The more common alternative is to add Examples sections to the API docs. This doesn't work well concerning findability though if the examples use more than one class. So in this case I think the approach by @jberlanga might be the best option.

One thing I like is if a package has a first code example that shows what it's about (and ideally does something cool / non-trivial) that I can read in less than one minute. If it's very short it could be on the front page of the docs, otherwise the getting started page. Currently the getting started page starts with a section general guidelines section that has longish examples (10+ lines showing how to instantiate a target and an observer).
That's fine, but can you please add a shorter example first (maybe computing a rise / set time and making a plot?)
Examples of what I like: requests, pyephem (code example is on the right of the front page).

Looking at the skyfield front page, the code examples are about RA/DEC to Alt/Az coordinate transformation, similar to what @eteq did in the observing example page in the Astropy docs.
@eteq – What's your preference here? Move that page to the Astroplan docs (and use the astroplan toolbox where it makes sense)? Or keep that page in the Astropy docs?

@jberlanga

Copy link
Copy Markdown
Member Author

Update: http://jberlanga.github.io/astroplan/ now matches latest commit (6c26eb5).

@jberlanga

Copy link
Copy Markdown
Member Author

@cdeil: Looks like putting in a .. code-block:: python directive causes the Travis CI build to try and run any code in the block below? See here.

@cdeil

cdeil commented Aug 10, 2015

Copy link
Copy Markdown
Member

Yes, we have doctests enabled for astroplan (see option here).

In principle this is a good thing, to have the examples from the docs tested.
But I have to admit that it can be painful to get them to work.

For the cases where you can't get a docs example to work, or where making it work would mean adding several lines of setup code that's too distracting for the docs, there's flexible options to skip code example lines or blocks or whole files:
http://astropy.readthedocs.org/en/latest/development/testguide.html#skipping-doctests

@jberlanga

Copy link
Copy Markdown
Member Author

@astroplanners/astroplanners-contributors

Hi all, another major update to the docs: See plotting tutorial page.

A couple things I still need to fix:

  • The images are all cut off on the right/top/bottom.
  • Not sure how to refer to astroplan's method for calculating airmass in the "See also" box just above this section, as I can't seem to find this in the API. @bmorris3: any suggestions?

@jberlanga

Copy link
Copy Markdown
Member Author

@astroplanners/astroplanners-contributors

Hi everyone,

This is nearing completion on my end (see jberlanga.github.io/astroplan/), but there is the matter of the last two Travis CI builds failing with the error: urllib2.URLError: <urlopen error [Errno -3] Temporary failure in name resolution>. Does anyone know what this might be about?

@bmorris3

Copy link
Copy Markdown
Contributor

@jberlanga My PRs have been doing that too. I don't know what it is, but I think we could use some advice from @astroplanners/astroplanners-contributors .

@cdeil

cdeil commented Aug 21, 2015

Copy link
Copy Markdown
Member

The error message (e.g. https://travis-ci.org/astroplanners/astroplan/jobs/76501136#L531) is pointing to this line: https://github.com/astroplanners/astroplan/blob/master/astroplan/core.py#L20

So the problem is this call

iers.IERS.iers_table = iers.IERS_A.open(download_file(iers.IERS_A_URL,
                                                      cache=True))

I don't understand in detail what the issue is. Maybe the file really can't be downloaded at the moment and it only works for you locally because you have it in your cache? Or maybe travis-ci doesn't allow internet access at this point of execution?

Really downloading a file on import is a bad idea and we should try to find a better solution ... let's discuss in #15?

@jberlanga

Copy link
Copy Markdown
Member Author

@astropy/astroplan-maintainers -- Please have a look. This PR is in its final review.

@ejeschke

Copy link
Copy Markdown
Contributor

Looking good for 0.1 👍

@cdeil

cdeil commented Aug 28, 2015

Copy link
Copy Markdown
Member

@jberlanga – Thanks for this big piece of work!

I'm merging this now.

Of course this is just the starting point for the astroplan docs ... if you wanted to make further suggestions here, please file a new issue or better yet, make a pull request.

cdeil added a commit that referenced this pull request Aug 28, 2015
@cdeil
cdeil merged commit 113a846 into astropy:master Aug 28, 2015
@cdeil

cdeil commented Aug 28, 2015

Copy link
Copy Markdown
Member

I think the HTML build worked on treadthedocs:
http://astroplan.readthedocs.org/en/latest/tutorials/plots.html

The PDF build failed (but we can ignore that for now):
https://readthedocs.org/projects/astroplan/builds/3280359/

@eteq

eteq commented Aug 28, 2015

Copy link
Copy Markdown
Member

Oops, @cdeil, this was still under review - @adrn, @jberlanga and I were going to have a telecon about this today in the early afternoon and then merge after. Now that I look, though, that wasn't noted here on the PR (we were talking about it over e-mail...), so I guess you couldn't have known that.

At any rate, @jberlanga, it'll probably be easiest if I leave some comments in this PR since it has the diff, and then you can issue a new PR with those corrections? (I may just do some of them myself if it turns out that's easier).

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.

6 participants