-
Notifications
You must be signed in to change notification settings - Fork 272
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
Example notebooks in Sphinx documentation #941
Example notebooks in Sphinx documentation #941
Conversation
…nto feature/notebooks-in-docs
Codecov Report
@@ Coverage Diff @@
## master #941 +/- ##
======================================
Coverage 78.6% 78.6%
======================================
Files 191 191
Lines 10910 10910
======================================
Hits 8576 8576
Misses 2334 2334
Continue to review full report at Codecov.
|
- made some modifications to table_writer_reader so that no cells fail (even ones that are supposed to)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is great work and should be merged.
I have some minor comments.
docs/tutorials/ctapipe_handson.ipynb
Outdated
"metadata": {}, | ||
"outputs": [], | ||
"source": [ | ||
"data = utils.get_dataset_path(\"gamma_test.simtel.gz\")\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the return value of get_dataset_path
is a path-like or string pointing to a file containing data .. it is not the data itself. Therefore I think this should be named path
docs/tutorials/ctapipe_handson.ipynb
Outdated
"cell_type": "markdown", | ||
"metadata": {}, | ||
"source": [ | ||
"note that this is (N_chan, N_pix, N_samp)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The output of Box 7 uses this terminology: n_channels x n_pixels, n_samples
. I think a few more letters might not hurt here. Also I always wondered: why does the container help use a an x
between the first two dimensions and a comma ,
between the last two?
docs/tutorials/ctapipe_handson.ipynb
Outdated
"metadata": {}, | ||
"outputs": [], | ||
"source": [ | ||
"data = utils.get_dataset(\"gamma_test_large.simtel.gz\") \n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deprecated: --> get_dataset_path
"source": [ | ||
"import pandas as pd\n", | ||
"\n", | ||
"hillas = pd.read_hdf(\"hillas.h5\", key='/dl1/hillas')\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assumed, somebody just sent me hillas.h5
via email .. how would I know the key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe one could show a bit how dataset exploration works in ctapipe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, i think many of these noteboooks will need improvement, for now I'm just moving them over from the old dir and not making large mods other than bug fixes.
To look inside a hdf5 file there are many options h5ls
probably is the easiest
Nice work. While I didn't look at each notebook in detail I think its a good idea 👍 |
Ok, I think this PR is ready to go - I've entirely removed the old examples/notesbooks dir and migrated everything over to the documentation. I'm not going to do many changes to the notebooks in this PR - we can have another to clean them up more, here I'm just focusing on getting them to render correctly. Their content probably needs a second look later. The next step (for another PR) will be to clean up the non-notebook examples (probably turn some into notebooks and others in to Tools) |
deprecated - now uses nbsphinx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! Let's merge it once tests are passing.
Ok all tests finally pass, so I just need some reviewers |
This PR makes it possible to add notebooks into the documentation directly using
nbsphinx
. It moves some of the current examples/notebooks intodocs/examples
anddocs/tutorials
. I also reorganized a bit the documentation tree (to put the API docs at a lower-level)The features are:
This is still a bit of a work in progress, as I need to migrate more of the notebooks to this new scheme (it takes some re-editing), but I think already it's useful.