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

Converted NEST by example to Jupyter notebook and updated for 2.12. #602

Merged
merged 15 commits into from
Jan 17, 2017

Conversation

hakonsbm
Copy link
Contributor

Converted NEST by example to Jupyter notebook. Also made sure that it works with 2.12, as specified in #558.

It is possible to convert the notebook to pdf. I did not get nbconvert --to pdf to work with references however, so it has to be converted to latex first, then one can use bibtex and pdflatex to make a pdf:

> jupyter nbconvert --to latex --template latex_template.tplx <filename.ipynb>
> pdflatex <filename.tex>
> bibtex <filename>
> pdflatex <filename.tex>
> pdflatex <filename.tex>

When converting to a pdf, authors, abstract, and proper references are inserted. This is specified in the template-file latex_template.tplx.

@heplesser heplesser added ZC: Documentation DO NOT USE THIS LABEL I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) ZP: PR Created DO NOT USE THIS LABEL S: Normal Handle this with default priority T: Enhancement New functionality, model or documentation labels Jan 2, 2017
@heplesser heplesser added this to the NEST 2.12 milestone Jan 2, 2017
@Silmathoron
Copy link
Member

Hi @hakonsbm, I started reviewing your PR but there are a lot more file-changes than what is stated in your description... is this normal? What is the relation between your PR and #580?

@hakonsbm
Copy link
Contributor Author

hakonsbm commented Jan 4, 2017

@Silmathoron When updating for 2.12 I had to make sure that the examples didn't raise any deprecation warnings. This meant that the branch had to include changes from #580. But now that #580 is merged into master I have updated the branch, and files changed in #580 no longer show up here.

},
"outputs": [],
"source": [
"nest.PrintNetwork(2)"
Copy link
Member

Choose a reason for hiding this comment

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

Under Python 3, with current master, this returns None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Silmathoron The PrintNetwork function has no return value. It only outputs the network tree to the terminal you used to start Jupyter notebook.

Copy link
Member

Choose a reason for hiding this comment

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

My bad, should have read what was written below a little more carefully...
Ok! Everything is working under py3, then, I'll start reading the text ASAP ;)

Copy link
Contributor

@stinebuu stinebuu left a comment

Choose a reason for hiding this comment

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

This looks mostly good to me. I have made some rephrasing suggestions, and found some typos.

"cell_type": "markdown",
"metadata": {},
"source": [
"# Introduction\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there should be a headline at the beginning of the notebook, "NEST by Example - An Introduction to the Neural Simulation Tool NEST Version 2.12". I know it is written at the top when you open Jupyter notebook, but the notebook starts quite abruptly when it starts right on 'Introduction'.

"simulated, how simulations can be run in parallel, using multiple\n",
"cores or computer clusters, and how parts of a model can be\n",
"randomized.\n",
"\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

This paragraph (lines 20-25) should be removed. You say the same in the paragraph above and the paragraph below.

"simulator (for the Blue Brain Project: <cite data-cite=\"Migliore06_119\">Migliore et al., 2006</cite>) and\n",
"IBM's C2 simulator <cite data-cite=\"Ananthanarayanan09\">(Ananthanarayanan et al. 2009)</cite>.\n",
"\n",
"Today, in 2012, there are several simulators for large spiking\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

2017

" \n",
"In the next sections, we will illustrate how to use NEST, using\n",
"examples with increasing complexity. Each of the examples is\n",
"self-contained. \n",
Copy link
Contributor

Choose a reason for hiding this comment

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

The PDF has a line that says 'Additionaly, you can find all examples in your NEST distribution.' Maybe keep that line in case people want to run them from the source or something.

"A node appears in the source position of `Connect` if it sends events\n",
"to the target node. In our example, the sine generator is in the\n",
"source position because it injects an alternating current into the\n",
"neuron. The voltmeter is in the source position, because it polls the\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the comma after source position.

" \\end{equation*}<br></td>\n",
" </tr>\n",
" <tr>\n",

Copy link
Contributor

Choose a reason for hiding this comment

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

have

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where is this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might be line 929, and that I wanted you to change 'having' to 'have'. But I can't be sure now, so I think it might be okay to leave it be.

" \\end{equation*}<br></td>\n",
" </tr>\n",
" <tr>\n",

Copy link
Contributor

Choose a reason for hiding this comment

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

Why three lines ---?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where?

Copy link
Contributor

Choose a reason for hiding this comment

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

Line 1372

" \\end{equation*}<br></td>\n",
" </tr>\n",
" <tr>\n",

Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove 'the' before 'Example 1'.

" \\end{equation*}<br></td>\n",
" </tr>\n",
" <tr>\n",

Copy link
Contributor

Choose a reason for hiding this comment

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

The first function from the code above is also the first one...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where is this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Line 1995.
I realize my comment isn't the best one, sorry about that. I think you should switch 'above' and 'code' from the original sentence, and remove 'we look at', so that the sentence becomes 'The first function from the code above is also the first one that is called for every class object.'. But this is just a suggestion :)

" \\end{equation*}<br></td>\n",
" </tr>\n",
" <tr>\n",

Copy link
Contributor

Choose a reason for hiding this comment

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

I would write an alternative version.

Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

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

@hakonsbm This looks generally very fine. But you should shorten the file name to "NEST_by_Example.ipynb". Also, it seems that even though you have defined the template file with abstract, authors, etc, this is not included in exported PDF, and the "Nordlie tables" are not rendered properly in PDF, at least when exporting to PDF via "Download as PDF via LaTeX". Is there any way to fix this?

\cite{Gewa:2012(533)} and has been updated for NEST 2.12.

\begin{description}
\item[Updated to 2.12.0] Håkon\ Mørk, December 2016
Copy link
Contributor

Choose a reason for hiding this comment

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

You could add here "Converted to Jupyter Notebook and updated ..."

"\n",
"<table class=\"image\">\n",
"<caption align=\"bottom\">The network consists of three populations: $N_E$ excitatory neurons (circle labeled E), $N_I$ inhibitory neurons (circle labeled I), and a population of identical, independent Poisson processes (PGs) representing activity from outside the network. Arrows represent connections between the network nodes. Triangular arrow-heads represent excitatory and round arrow-heads represent inhibitory connections. The numbers at the start and end of each arrow indicate the multiplicity of the connection.</caption>\n",
"<tr><td><img src=\"figures/brunel_detailed_external_single2.jpg\" alt=\"Brunel detailed network\" style=\"width: 500px;\"/></td></tr>\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

I would maybe remove the "width" specification. Or is there a way to specify that you want a width of at most 500px?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is indeed a way to limit the size of the figure. But there is maybe no need for that here, so I can remove it.

" \\end{equation*}<br></td>\n",
" </tr>\n",
" <tr>\n",

Copy link
Contributor

Choose a reason for hiding this comment

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

I would update this to "refer to NEST documentation at [www.nest-simulator.org](http://www.nest-simulator.org/documentation/)"

" \\end{equation*}<br></td>\n",
" </tr>\n",
" <tr>\n",

Copy link
Contributor

Choose a reason for hiding this comment

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

Are these real rates??? They seem too "perfect"!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where do you refer to here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@hakonsbm Commeting ipynb in Github is not straightforward ;). The lines I meant are rather 957-958 in the cell with execution count 9:

     "Excitatory rate   : 20.00 1/s\n",
      "Inhibitory rate   : 20.00 1/s\n"

I just find it surprising that the simulation here should have resulted in rates of exactly 20.00 for both excitatory and inhibitory neurons.

@hakonsbm
Copy link
Contributor Author

@stinebuu @heplesser Comments made on lines after line 493 are apparently not placed correctly, so I'm unsure about what you refer to in some of them.

@hakonsbm
Copy link
Contributor Author

hakonsbm commented Jan 13, 2017

@heplesser When exporting to PDF via "Download as PDF via LaTeX" it skips the template file completely. Therefore authors, abstract, and tables are missing when creating a PDF this way (the template substitutes the HTML tables with their LaTeX counterparts). For this reason exporting has to be done using the command line: first exporting to LaTeX using the template, then using pdflatex and bibtex to create the PDF.

I can include a small shell script that does the conversion and the clean up, if you want?

Also added the network model figure to the LaTeX conversion.
@heplesser
Copy link
Contributor

@hakonsbm A shell-script for the conversion would be useful, and the PDF created by it should also be committed. Then, all the old stuff (bib and latex files, scripts) should be removed, so that only the ipynb, the PDF generated from it, and the files required by the ipynb and for turning it into a nice PDF should remain.

Copy link
Contributor

@stinebuu stinebuu left a comment

Choose a reason for hiding this comment

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

Good job @hakonsbm ! I have nothing more I want changed. 👍

@heplesser
Copy link
Contributor

@hakonsbm Thanks! I tidied up a little more, see the PR I created against your branch. Once you have merged that, this one will be ready to merge.

@hakonsbm
Copy link
Contributor Author

@heplesser Thank you! It is merged now.

Conflicts:
	doc/nest_by_example/scripts/one_neuron_with_sine_wave.py
@heplesser
Copy link
Contributor

@hakonsbm Why did you remove the one_neuron_with_sine example? This seems entirely unrelated to this PR.

@hakonsbm
Copy link
Contributor Author

@heplesser It was removed with your PR against my branch, but it had been changed in master, causing conflicts here. So I had to merge master into this branch and remove it again.

But if it shouldn't have been removed I can restore it.

@heplesser
Copy link
Contributor

@hakonsbm Sorry, I got confused. You were right to remove the file.

@heplesser heplesser merged commit f2e9b51 into nest:master Jan 17, 2017
@hakonsbm hakonsbm deleted the nest_by_example branch January 17, 2017 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Enhancement New functionality, model or documentation ZC: Documentation DO NOT USE THIS LABEL ZP: PR Created DO NOT USE THIS LABEL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants