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

Convert UVES tutorial to ipython notebook format #43

Merged
merged 27 commits into from
Oct 6, 2014

Conversation

migueldvb
Copy link
Member

Pasted UVES tutorial into an ipython notebook. The cells that nteractively hide/show part of the code in the exercises are not yet implemented in the notebook.

@eteq
Copy link
Member

eteq commented Feb 14, 2014

Quick suggestion from a glance: can you change the download to use the astropy download tools? This will add both caching and a progress bar The first code cell would then look like this:

import tarfile
from astropy.utils.data import download_file
url = 'http://python4astronomers.github.io/_downloads/astropy_UVES.tar.gz'
tarfile.open(download_file(url, cache=True), mode='r|*').extractall()

Also, what's going on with the cd and ls in the tutorial? Those look like shell commands rather than python? When in the ipython notebook style, I think it might be better to use tarfile to directly access the file contents of that tarball, instead of extracting it and then accessing the data - that would leave a data directory wherever the user might run the notebook (a subtlety likely to be lost on a new user)

@migueldvb
Copy link
Member Author

Thanks @eteq. I have used the download_file function to download the data and removed the shell commands. The script still extracts the contents of the tarfile to disk in the directory where notebook is run. Actually I think that this could be more intuitive for a new user that may want to open the files with a FITS viewer.

@eteq
Copy link
Member

eteq commented Feb 14, 2014

Hmm, perhaps... But in the notebook format, it's really not obvious "where" the user is (e.g. what directory). Perhaps extracting to an explicit location is better? Something like:

import tarfile
from astropy.utils.data import download_file
url = 'http://python4astronomers.github.io/_downloads/astropy_UVES.tar.gz'
f = tarfile.open(download_file(url, cache=True), mode='r|*')

location_for_data_file = '.'  #CHANGE TO WHEREVER YOU WANT THE DATA TO BE EXTRACTED
f.extractall(location_for_data_file)

Then at least the user knows exactly where it's going?

Alternatively, you could have the tutorial work without extracting the file, but somewhere you could say "If you wish to extract the file to look at it with a fits image viewer, just to f.extractall(location)".

There's also the prosaic problem that this will extract the data file every time someone deploys the notebook.... Not necessarily a serious problem, but possibly mildly annoying for the maintainers.

@adrn, what do you think?

@migueldvb
Copy link
Member Author

I have used @eteq's suggestion to define a directory path to extract the data in the last commit until this issue is settled.

@eteq
Copy link
Member

eteq commented Feb 16, 2014

Looks fine to me now - @adrn, have any opinions here?

@adrn
Copy link
Member

adrn commented Feb 16, 2014

Hm, that seems fine.

@migueldvb Looking good! Can you remove the sphinx markup from the text? Anywhere you see something like :ref: can be turned into a link, and anywhere you see :math: can be turned into latex (just wrap the math stuff with dollar signs).

@migueldvb
Copy link
Member Author

Thanks @adrn, I'm starting to correct those formatting issues.

For references to other tutorials should I use absolute or relative URLs?

@eteq
Copy link
Member

eteq commented Apr 17, 2014

@migueldvb - I think relative URLs make the most sense, as that way we don't have to worry about anything odd that might come from changing registrars and the like.

@migueldvb
Copy link
Member Author

@eteq That sounds good. I think that all the references have been fixed in commit 26c5e8c

@adrn
Copy link
Member

adrn commented Apr 17, 2014

Here's a direct link:
http://nbviewer.ipython.org/urls/raw.githubusercontent.com/migueldvb/astropy-tutorials/uves/tutorials/UVES/UVES.ipynb?create=1

  • Still some lingering Sphinx markup, e.g., :math:
  • A broken link in "Scientific background"
  • We'll probably need to include some of the other files, or rework the text because it currently references spectra_utils.py

@migueldvb
Copy link
Member Author

@adrn I have fixed the first two items in the list in the commits b8245e3 and 4af6d49. I'm not sure where
the file spectra_utils.py could be found. Maybe just adding a link to this file in the tutorial should be fine?

@adrn
Copy link
Member

adrn commented Apr 21, 2014

Every time I look I find another piece of Sphinx markup :)

In Acknowledgements, there is :Authors: and :Copyright: -- should probably just do a regex search for anything between two colons...

@adrn
Copy link
Member

adrn commented Apr 21, 2014

Hm I also notice a few formatting issues -- are you editing the .ipynb file directly, or through the notebook? Some cells contain text and code, and some code is not properly indented. Could you open in the notebook and review the cells one by one?

One other Sphinx relic I've noticed -- some lines end in double colons, but they should just be single colons now.

@migueldvb
Copy link
Member Author

I think that all of those formatting issues have been corrected now.

@taldcroft
Copy link
Member

@migueldvb - You need to include proper CC license information and attribution to Smithsonian Astrophysical Observatory: http://python4astronomers.github.io/#license

And probably a good idea to let @hamogu know that his chapter is going new places! :-)

@eteq
Copy link
Member

eteq commented May 19, 2014

@migueldvb - the notebook now encompasses everything in UVES.py and UVES.rst, right? If so, you may as well just delete those files so there's no confusion.

Also, the metadata no longer lives in a separate metadata.cfg file, but instead as metadata at the top of the notebook JSON (your notebook already has a nearly empty metadata section at line 2, you'll see). E.g., https://github.com/astropy/astropy-tutorials/blob/master/tutorials/Quantities/Quantities.ipynb#L2
You can either modify the file directly, or if you're on IPython 2.0 or later, there's an "edit metadata" button that should work.

"cell_type": "markdown",
"metadata": {},
"source": [
"The previous astropy tutorial already covered\n",
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps "The previous ..." -> "A previous ..."

@eteq
Copy link
Member

eteq commented May 19, 2014

@migueldvb - I think it makes sense for you to add yourself to the list of authors here (as long as @hamogu has no objection).

(And one other thing you may or may not know: to see how the final version looks, you can do python prepare_deploy.py run convert, and it will create the tutorial in an html directory. That does require you to update the metadata as I described above, though.)

@hamogu
Copy link
Member

hamogu commented May 19, 2014

@migueldvb - Since you authored on this notebook, you should be listed
as an author.

@hamogu
Copy link
Member

hamogu commented May 19, 2014

@taldcroft : Everything should have a licence and CC is fine, if that's what we decided to use for the tutorial notebooks.
I don't think an attribution to the Smithsonian Astrophysical Observatory (SAO) is technically required here (since I am not an employee and don't receive a salary, SAO does not necessarily own the copyright, even if I had written the original tutorial in my "office time"); my understanding is that the original copyright is with me and not SAO. On the other hand, I am perfectly willing to transfer the copyright of the original version to SAO as a way to attribute who paid for the work on MN Lup, that forms the base of this tutorial.</nit-picky detail>

"cell_type": "markdown",
"metadata": {},
"source": [
"Then start up IPython with the ``--pylab`` option to enable easy plotting"
Copy link
Member

Choose a reason for hiding this comment

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

Since we now deliver this as an IPython notebook, IPython must be started already at this point (or how do you read the notebook in the first place?).
Maybe here should be a link to "inline plotting in IPython"or something like that?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that this sentence can be removed since one can use the inline backend for plotting

@migueldvb
Copy link
Member Author

@taldcroft, @hamogu - I have included the CC lincense information in commit ba520c9

@migueldvb
Copy link
Member Author

rebasing on top of the master branch

@migueldvb
Copy link
Member Author

@eteq I get a KeyError when I run the command: python prepare_deploy.py run convert:

[NbConvertApp] Converting notebook /home/mdevalbo/Source/astropy-tutorials/tutorials/UVES/_run_UVES.ipynb to html
[NbConvertApp] Support files will be in /home/mdevalbo/Source/astropy-tutorials/html/UVES_files/
Traceback (most recent call last):
  File "prepare_deploy.py", line 195, in <module>
    convert_notebooks(args.nameregex)
  File "prepare_deploy.py", line 145, in convert_notebooks
    index_listing["description"] = nb['metadata']['astropy-tutorials']['description']
KeyError: 'description'

@migueldvb
Copy link
Member Author

@eteq Now I only get warnings when I run the command

python prepare_deploy.py run convert -n UVES -q

I'm not sure how to ger rid of those.

"License: This tutorial is made available under the terms of the\n",
"[Creative Commons Attribution 3.0 License](http://creativecommons.org/licenses/by/3.0/)\n",
"\n",
"Authors: Moritz Guenther\n",
Copy link
Member

Choose a reason for hiding this comment

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

You should update this with your name too, @migueldvb, so that you get proper credit!

@eteq
Copy link
Member

eteq commented Sep 5, 2014

@migueldvb - what warnings are you seeing? I just tried it now and saw no warnings. So other than those two small things I mentioned, I think this may be all set. Or perhaps @hamogu wants to take a final look at it?

@hamogu
Copy link
Member

hamogu commented Sep 6, 2014

I won't have time to look at this again in the next few weeks, so please
merge when you think it's ready.

@migueldvb
Copy link
Member Author

Thanks @eteq, I have made those changes.

I think that the only remaining warning appears when trying to extract the downloaded tarball in a location where the data has already been extracted. I will check later this week because I am missing a dependency to run the prepare_deploy.py script on my laptop.

@migueldvb
Copy link
Member Author

There is a FITSFixedWarning after the WCS transformation because of a non-standard FITS keyword:

WARNING: FITSFixedWarning: RADECSYS= 'FK5 ' / Coordinate reference frame 
RADECSYS is non-standard, use RADESYSa. [astropy.wcs.wcs]
WARNING:astropy:FITSFixedWarning: RADECSYS= 'FK5 ' / Coordinate reference frame 
RADECSYS is non-standard, use RADESYSa.

Other than that I think that the other issues have been fixed now.

@astrofrog
Copy link
Member

I think it would make sense to use standard-compliant files for tutorials to avoid this kind of issue. This one can be fixed by just changing RADECSYS to RADESYS.

@migueldvb
Copy link
Member Author

@astrofrog That makes sense. How can the tarfile be updated in the python4astronomers download area?

@astrofrog
Copy link
Member

Either we can update it via the python4astronomers repository:

https://github.com/python4astronomers

or we could consider hosting it ourselves as part of astropy. Maybe this is a good chance to finally make use of data.astropy.org (cc @eteq)

@migueldvb
Copy link
Member Author

Thanks @astrofrog. I have renamed the keyword in the UVES data and made a PR in the python4astronomers repo: python4astronomers/python4astronomers.github.com/pull/3

@eteq
Copy link
Member

eteq commented Sep 11, 2014

I like @astrofrog's suggestion of switching this to using data.astropy.org. @astrofrog, If I understand correctly, you have to do this because it's currently mirroring your server. So can you grab the .tar.gz file that @migueldvb has in python4astronomers/python4astronomers.github.com#3 , and place it at tutorials/UVES/data_UVES.tar.gz of http://www.mpia-hd.mpg.de/~robitaille/astropy/data/? Then if you hit the URL Mark sent us via e-mail, it should get copied over, right?

Once that's done, @migueldvb, you can just change the url variable in the first code cell to 'http://data.astropy.org/tutorials/UVES/data_UVES.tar.gz' and it should all work the same (with your warning fixed!). If that works, then I think this is good to merge!

* Switching to data.astropy.org
@astrofrog
Copy link
Member

The data should be on http://data.astropy.org/tutorials/UVES/

@eteq
Copy link
Member

eteq commented Sep 28, 2014

@astrofrog - just tried it now and it didn't work, I think because it's astropy_UVES.tar.gz on data.stropy.org, but the notebook is expecting data_UVES.tar.gz. Can you rename it easily, or should we change the notebook to be astropy_UVES.tar.gz? (I don't like that name because it's not obvious that's the data file if it just says "astropy")

@eteq
Copy link
Member

eteq commented Oct 6, 2014

Ok, @astrofrog updated the file, so this is good to go!

I noticed a couple other which should probably be changed while testing, and will just fix those directly as part of the merge process. Thanks, @migueldvb and @hamogu !

@eteq eteq merged commit eab6067 into astropy:master Oct 6, 2014
eteq added a commit that referenced this pull request Oct 6, 2014
Convert UVES tutorial to ipython notebook format
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants