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

add script to execute all the ipynbs in the examples/notebooks folder #724

Merged
merged 17 commits into from
Apr 25, 2018

Conversation

dneise
Copy link
Member

@dneise dneise commented Apr 24, 2018

reading the discussion in #716 made me want to test our jupyter notebooks.

In this PR I make a first proposal, I do not like it myself too much, but I thought maybe this sparks a discussion.

What I propose to do is, convert the ipynb files to python scripts and then simply execute them and see if the return value is zero or not.

In this first version I swallow all the stderr and stdout, since all of the scripts are very verbose. And I saw no nice way to get some overview.

I assume one could still catch the stderr and in case the return value is not zero, I could still try to find the last traceback and print it ...

The main problem is, %matplotlib inline, which is not working in a ipython session. So all our ipynbs which to %matplotlib inline fail this test of mine, even when they are perfectly valid.


A much better alternative might be this: https://github.com/zonca/pytest-ipynb
but it needs us to rewrite our notebooks a little...

Anyway I'm curious what you think?

@dneise
Copy link
Member Author

dneise commented Apr 24, 2018

@maxnoe
Copy link
Member

maxnoe commented Apr 24, 2018

This is great! Will hopefully preventlying examples/docs. However I think we should not silence stderr just stdout.

@maxnoe
Copy link
Member

maxnoe commented Apr 24, 2018

You could also execute directly using:

$ jupyter nbconvert --to notebook --execute mynotebook.ipynb

http://nbconvert.readthedocs.io/en/latest/execute_api.html

@dneise
Copy link
Member Author

dneise commented Apr 25, 2018

Thanks @maxnoe ! I did not know about --execute and was unable to find it. The script is now much shorter and it can execute correctly all the plotting stuff.

@dneise
Copy link
Member Author

dneise commented Apr 25, 2018

On my laptop I ran into this issue:
jupyter/notebook#3407

and I've seen the same on travis. So I decided to follow the fix mentioned in that issue above and downgraded to tornad0 4.5.2.

@watsonjj
Copy link
Contributor

watsonjj commented Apr 25, 2018

This is nice. Just a thought: Could you put this into a pytest file so its automatically ran by pytest anyway in travis? That would have the benefit of automatically indicating to the user locally if they need to change the notebooks for their changes. Would that work?

@dneise
Copy link
Member Author

dneise commented Apr 25, 2018

List of remaining issues:

table_writer_reader.ipynb - ZeroDivisionError: division by zero

This notebook is full of exceptions, which are thrown by design. These exceptions are thrown in order to show how something behaves in case an exception is thrown.

I understand this, but I do not know how to deal with this ...

HDF5 Images.ipynb - NameError: name 'get_dataset_path' is not defined

-- fixed

calibrated_data_exploration.ipynb - AttributeError: 'DL1CameraContainer' object has no attribute 'waveform'

-- fixed

check_calib.ipynb - FileNotFoundError: [Errno 2] No such file or directory: '/Users/kosack/Data/CTA/Prod3/Old/gamma_20deg_180deg_run1000___cta-prod3-demo_desert-2150m-Paranal-demo2rad_cone10.simtel.gz'

-- can we add this file to ctapipe-extra?

HDF5 Images in TableDataset.ipynb - TimeoutError: Cell execution timed out

-- fixed: 30 events take ~1minute on my machine. maxevents was 3000. I reduced it to 30.

@dneise
Copy link
Member Author

dneise commented Apr 25, 2018

Just a thought: Could you put this into a pytest file so its automatically ran by pytest anyway in travis? That would have the benefit of automatically indicating to the user locally if they need to change the notebooks for their changes. Would that work?

I've got no idea yet, how to put this nicely into pytest... But you are right, I myself am sure I will forget to execute this script... I barely remember to pytest locally.

I believe we should maybe try out these https://github.com/zonca/pytest-ipynb at some point. This integrates nicely with pytest. But this means potentially heavy modifications our notebooks, which might not be feasible at the moment.

@kbruegge
Copy link
Member

Hello.

I like the idea of testing notebooks. 👍

There is also this little library which looks interesting to me: https://github.com/opengeophysics/testipynb I've never used it myself though.

Dominik Neise added 5 commits April 25, 2018 10:07
 - allow to exclude known failing notebooks (xfail)
 - print the typical pytest dots while testing (dot-line)
 - print the stderr of failing tests (and xfails) at the end
@dneise
Copy link
Member Author

dneise commented Apr 25, 2018

Okay ... this is I think the end from my side. I have not more time to spend on this matter. So I would like to give it into the hands of the community now.

I added a way to skip tests of notebooks, which are known to fail (similar to pytests xfail).

So now I hope the build will pass even with the two notebooks which are still failing.
If the build passes .. I hope we can maybe merge this even though there are still a huge number of features missing here ...

@codecov
Copy link

codecov bot commented Apr 25, 2018

Codecov Report

Merging #724 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #724   +/-   ##
=======================================
  Coverage   68.14%   68.14%           
=======================================
  Files         196      196           
  Lines       10546    10546           
=======================================
  Hits         7187     7187           
  Misses       3359     3359

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b904902...1dfb858. Read the comment docs.

@dneise
Copy link
Member Author

dneise commented Apr 25, 2018

okay tests are passing and output seems reasonable to me:
https://travis-ci.org/cta-observatory/ctapipe/jobs/370960158#L1956-L1967

@dneise
Copy link
Member Author

dneise commented Apr 25, 2018

I have no idea what codacy is whining about again ...

@kosack
Copy link
Contributor

kosack commented Apr 25, 2018

pytest file so its automatically ran by pytest anyway in travis

we can just add it to .travis anyhow - if the script returns a non-zero value, it should get marked as failed.

@kosack
Copy link
Contributor

kosack commented Apr 25, 2018

I have no idea what codacy is whining about again ...

It thinks ctapipe is a python2.7 project, so always complains about syntax with error 999... I can't find anywhere in their docs where you can set the python version, except one place where you set it for pyflakes, and I did that, but nothing changed.

So far, I've been disappointed in landscape.io (constantly failed to process pull requests), and codacy is better but has these problems. ANyhow, this PR looks fine

@kosack kosack merged commit fc74c23 into cta-observatory:master Apr 25, 2018
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.

5 participants