Conversation
…essModel Signed-off-by: Risbud, Sumedh <[email protected]>
- uses fixed point bit-accurate ProcessModels for LIF and Dense
- resets internal neural state of all LIF neurons
- these changes are needed to make the pre-trained networks parameters work, because the network was trained with these assumptions
Signed-off-by: Risbud, Sumedh <[email protected]>
awintel
left a comment
There was a problem hiding this comment.
This sentence seems unnecessarily cryptic:
"Between those leaning towards Neuroscience and those partial to Computer Science, this tutorial aims to be appealing to the former."
What or who is "those"? What talk in such strange terms of 'Neuroscience' and 'Computer Science'?
Just state what the purpose of this tutorial is: To give an example of building a hierarchical Lava process from other processes that solves an actual task like classification.
Regarding your "Important Note:"
Why tell this whole backstory about SNN-TB and that the accuracy dropped but we actually did not do anything about it?
It's just a simple 4 layer MLP for classifying MNIST. Since the focus of this tutorial is on how to build such a multi-process/layer network and not how to train classifiers, we simply load a set of pre-trained weights that gives 80% sufficient for this POC. Refer people to lava-dl for actually DNN training and other models.
Regarding: "The 3 Processes shown above are:"
Why not write the processes in the following bullets the way you have written them in the box diagram? SpikeInput, ImageClassifier, (Spike)Output
Also why name them again differently in this section: "Create the Process class"?
SpikeInput, MnistClassifier, OutputProcess?
Seems all a bit inconsistent.
In the "PySpikeInputModel" and "PyOutputProcessModel" ProcModels you should not be using class variables for what actually are probably meant to be instance variables. Yes it will work in this case but we should not encourage people to such practices which are prone to side effects.
Here "# Reset internal neural state of LIF neurons", it's probably best to implement broadcasting to the shape and type of the var in the set(..) method rather than writing such code repeatedly:
mnist_clf.lif1_u.set(np.zeros((64,), dtype=np.int32))
It could be as simple as:
mnist_clf.lif1_u.set(0)
A simple type and shape check in set() should do it.
What is the ".astype" good for?
ground_truth = output_proc.gt_labels.get().astype(np.int32)
Can't this be done without converting it to int32?
accuracy = np.sum(ground_truth==predictions)/ground_truth.size * 100
I would not go as far as calling a single method an application programming interface:
"Var.set() API"
Nah...
"But that approach is perhaps a bit advanced for the scope of this tutorial."
Either use RefPorts (which would be appropriate here) when fixed as an example when remote memory access might be useful in a practical example like this or simply just do it the way you do.
Thanks
phstratmann
left a comment
There was a problem hiding this comment.
Thanks, Summit, for your changes! I think that the tutorial has substantially improved and offers a great deep dive into the topics. Still, a few changes that I would suggest:
Please fix the url links.
I would argue against referring to the Process tutorial in the very beginning. The MNIST tutorial should be a deep dive and thus the first tutorial that anyone needs to go through (apart from how to install Lava). Thus, I would delete the reference here and include all information on processes that are really crucial into this tutorial.
Similarly, I would put the "Follow the links below for deep-dive tutorials" to the end. Otherwise, a user would feel forced to look at those tutorials first - but we want them to go through MNIST first.
I would rephrase the first "Important Note"s at the beginning and end. Or maybe even delete it? The user is not interested in hearing the full background, and we should not sound negative. State that this tutorial just shows how to build, compile, and run a functional model. To build models with higher accuracy, you can refer people to our work on Learning at the end. Don't make it sound too negative.
Figure:
- The blue boxes around "spikes", "activation" etc look confusing to me. They look like the blue boxes Latex sometimes puts around urls. Would go without them.
- "spike rates"
- Do you have 3 layers (as shown here) or 4 layers (as mentioned further down)?
- would be an idea to use the same var and ports names as further down in the actual code, e.g. "b_lif1" instead of "bias"
"# Assumes: $PYTHONPATH contains lava repository root" -> Isn't this explicitly stated in the how to install instructions? If so, leave it out.
"about how the Process [behave and] will be executed [by computing hardware]"
"The other phases are: pre-management, post-management, and learning."
This will need a brief explanation. What happens in each phase? Something like "In general, there are four phases... Out of these, we will here only use the spiking phase."
"# Guard function for PostManagement phase. The run_post_mgmt
function is executed only if the guard function returns True."
This description will not be clear to someone who is just starting to use Lava. What is the guard function? May be explained in the text above (see last comment)?
Your enumerations should be consistent:
- points or numbers?
- start with the title of the bullet point (such as "Spike Input Process") and don't put it at the end (like "(SpikeInput)")
In [3]:
- I think we use the convention that input variables are briefly defined at the beginning of a class docstring?
- wouldn't "shape" be an input argument?
- I would avoid "init=kwargs['vth']" and instead .pop vth at the beginning of init
"The code in these ProcessModels is what will get executed. Processes above were just declarations or interfaces, in a way."
Does not sound very elegant / clear. Maybe copy text on Processes and ProcessModels from the respective tutorials?
"# Import decorators" -> people will not know what a decorator is. Needs a brief sentence as explanation.
ProcessModel for the feed-forward network -> refer here to hierarchical models (and the tutorial about them)
"We take this approach to clear the neural states of all three LIF layers inside the classifier after every image. We need to clear the neural states, because the network parameters were trained assuming clear neural states for each inference."
Can't you re-set the states using the .set() function? (From reading the code: it seems that you actually do use the var.set() command to reset values. Thus, this sentence must be deleted?)
" In principle, the same function can be achieved by using RefPorts. But that approach is perhaps a bit advanced for the scope of this tutorial."
-> The second sentence is too informal. And if you want to mention RefPorts (which is great!) you need to briefly describe what the difference is between RefPorts and Var.set().
In [6]:
- what does it mean to create aliases? Will not be clear for a novice
Typos:
... bird's-eye view (only 1 hyphen)
... with THE SNNToolBox
... enumeration, point 1: integrate-and-fire dynamics. Point 2: "Feed-Forward Process"
... Below, we create
... Post-management phase -> minor letter
... time step -> without hyphen
... THE ProcessModel for inference output
... we need to clear the neural states because -> no comma before "because"
... for A fixed number of steps
Style:
- for headings, we decided to use one "#", two "##", and four "####". Please stick to this convention.
|
@awintel, @phstratmann I have addressed your comments and pushed the changed version. Please re-review. |
awintel
left a comment
There was a problem hiding this comment.
Looks fine now. But the whole story line should probably refreshed now that lava-dl exists at the end of the quarter.
|
Check this new issue as well before wrapping up this PR: |
phstratmann
left a comment
There was a problem hiding this comment.
Well done, thanks a lot for integrating my suggestions!
A few minor suggestions before I will approve the pull request:
"; compares the predicted class label with the ground truth"
Does the Output process really do that in the present tutorial? To my understanding, this only happens if you activate learning and is not implemented here.
I would briefly describe what the decorators do (1 sentence) or refer to a resource within Lava that does that. Most users will be unfamiliar with the concept, and the decorators used here will confuse them.
Formatting of term like "LoihiProtocol", "SubProcessModel", … in text: There are several processes and vars that you highlight using quotation marks (''). We previously decided to highlight these quantities using italic mode instead.
At several places, you describe what happens in a cell within the title:
- Finally, ProcessModel for inference output
- Create instances of the Processes and connect them
- Finally, ProcessModel for inference output
In my point of view, it would be more structured to have a single title indicating that in the next cells you will be "Creating ProcessModels for Python Execution", and then use markdowns to describe what happens in the individual cells without title font.
Please commit a Jupyter notebook where the cells have been cleared. Currently, I see the order in which you ran the cells as well as a KeyboardInterrupt error after the last cell.
I would write a last sentence indicating what the reader should see when he/she runs the last cell, and why this is cool.
Thanks for your great work!
Signed-off-by: Risbud, Sumedh <[email protected]>
Signed-off-by: Risbud, Sumedh <[email protected]>
* Minor: Removed a divide by zero warning from the fixed point LIF ProcessModel Signed-off-by: Risbud, Sumedh <[email protected]> * Improved to MNIST end-to-end tutorial - uses fixed point bit-accurate ProcessModels for LIF and Dense - resets internal neural state of all LIF neurons - these changes are needed to make the pre-trained networks parameters work, because the network was trained with these assumptions Signed-off-by: Risbud, Sumedh <[email protected]> * Post code review @awintel and @phstratmann Signed-off-by: Risbud, Sumedh <[email protected]> * Post code review @awintel and @phstratmann Signed-off-by: Risbud, Sumedh <[email protected]> * Post re-review by @phstratmann Signed-off-by: Risbud, Sumedh <[email protected]> Co-authored-by: PhilippPlank <[email protected]>
* Address review comments Signed-off-by: Marcus G K Williams <[email protected]> * Improve mnist tutorial (#147) * Minor: Removed a divide by zero warning from the fixed point LIF ProcessModel Signed-off-by: Risbud, Sumedh <[email protected]> * Improved to MNIST end-to-end tutorial - uses fixed point bit-accurate ProcessModels for LIF and Dense - resets internal neural state of all LIF neurons - these changes are needed to make the pre-trained networks parameters work, because the network was trained with these assumptions Signed-off-by: Risbud, Sumedh <[email protected]> * Post code review @awintel and @phstratmann Signed-off-by: Risbud, Sumedh <[email protected]> * Post code review @awintel and @phstratmann Signed-off-by: Risbud, Sumedh <[email protected]> * Post re-review by @phstratmann Signed-off-by: Risbud, Sumedh <[email protected]> Co-authored-by: PhilippPlank <[email protected]> * Check process lineage before join (#177) Signed-off-by: Marcus G K Williams <[email protected]> * Add NxSDKRuntimeService Signed-off-by: Marcus G K Williams <[email protected]> * Fix unit test, linting Signed-off-by: Marcus G K Williams <[email protected]> * Remove comments Signed-off-by: Marcus G K Williams <[email protected]> * Handle nxsdk import exception Signed-off-by: Marcus G K Williams <[email protected]> * Fix indentation issue Signed-off-by: Marcus G K Williams <[email protected]> * Uncomment board.run in nc proc model Signed-off-by: Marcus G K Williams <[email protected]> * Address review, rework NxSdkRuntime Service Signed-off-by: Marcus G K Williams <[email protected]> * Fix unit tests, merge with main Signed-off-by: Marcus G K Williams <Marcus G K Williams [email protected]> * Remove nc/ports.py again Remove commented code in compiler.py Signed-off-by: Marcus G K Williams <Marcus G K Williams [email protected]> * Update comments logging Signed-off-by: Marcus G K Williams <Marcus G K Williams [email protected]> * Update test Utils method name and document Signed-off-by: Marcus G K Williams <Marcus G K Williams [email protected]> * Update test name and docs for nxsdkruntimeservice Signed-off-by: Marcus G K Williams <Marcus G K Williams [email protected]> * Update docstrings for RuntimeService Signed-off-by: Marcus G K Williams <Marcus G K Williams [email protected]> * Update logging Signed-off-by: Marcus G K Williams <Marcus G K Williams [email protected]> * Remove unneeded logging import Signed-off-by: Marcus G K Williams <Marcus G K Williams [email protected]> Co-authored-by: Risbud, Sumedh <[email protected]> Co-authored-by: PhilippPlank <[email protected]> Co-authored-by: Marcus G K Williams <Marcus G K Williams [email protected]>
* Minor: Removed a divide by zero warning from the fixed point LIF ProcessModel Signed-off-by: Risbud, Sumedh <[email protected]> * Improved to MNIST end-to-end tutorial - uses fixed point bit-accurate ProcessModels for LIF and Dense - resets internal neural state of all LIF neurons - these changes are needed to make the pre-trained networks parameters work, because the network was trained with these assumptions Signed-off-by: Risbud, Sumedh <[email protected]> * Post code review @awintel and @phstratmann Signed-off-by: Risbud, Sumedh <[email protected]> * Post code review @awintel and @phstratmann Signed-off-by: Risbud, Sumedh <[email protected]> * Post re-review by @phstratmann Signed-off-by: Risbud, Sumedh <[email protected]> Co-authored-by: PhilippPlank <[email protected]>
* Address review comments Signed-off-by: Marcus G K Williams <[email protected]> * Improve mnist tutorial (lava-nc#147) * Minor: Removed a divide by zero warning from the fixed point LIF ProcessModel Signed-off-by: Risbud, Sumedh <[email protected]> * Improved to MNIST end-to-end tutorial - uses fixed point bit-accurate ProcessModels for LIF and Dense - resets internal neural state of all LIF neurons - these changes are needed to make the pre-trained networks parameters work, because the network was trained with these assumptions Signed-off-by: Risbud, Sumedh <[email protected]> * Post code review @awintel and @phstratmann Signed-off-by: Risbud, Sumedh <[email protected]> * Post code review @awintel and @phstratmann Signed-off-by: Risbud, Sumedh <[email protected]> * Post re-review by @phstratmann Signed-off-by: Risbud, Sumedh <[email protected]> Co-authored-by: PhilippPlank <[email protected]> * Check process lineage before join (lava-nc#177) Signed-off-by: Marcus G K Williams <[email protected]> * Add NxSDKRuntimeService Signed-off-by: Marcus G K Williams <[email protected]> * Fix unit test, linting Signed-off-by: Marcus G K Williams <[email protected]> * Remove comments Signed-off-by: Marcus G K Williams <[email protected]> * Handle nxsdk import exception Signed-off-by: Marcus G K Williams <[email protected]> * Fix indentation issue Signed-off-by: Marcus G K Williams <[email protected]> * Uncomment board.run in nc proc model Signed-off-by: Marcus G K Williams <[email protected]> * Address review, rework NxSdkRuntime Service Signed-off-by: Marcus G K Williams <[email protected]> * Fix unit tests, merge with main Signed-off-by: Marcus G K Williams <Marcus G K Williams [email protected]> * Remove nc/ports.py again Remove commented code in compiler.py Signed-off-by: Marcus G K Williams <Marcus G K Williams [email protected]> * Update comments logging Signed-off-by: Marcus G K Williams <Marcus G K Williams [email protected]> * Update test Utils method name and document Signed-off-by: Marcus G K Williams <Marcus G K Williams [email protected]> * Update test name and docs for nxsdkruntimeservice Signed-off-by: Marcus G K Williams <Marcus G K Williams [email protected]> * Update docstrings for RuntimeService Signed-off-by: Marcus G K Williams <Marcus G K Williams [email protected]> * Update logging Signed-off-by: Marcus G K Williams <Marcus G K Williams [email protected]> * Remove unneeded logging import Signed-off-by: Marcus G K Williams <Marcus G K Williams [email protected]> Co-authored-by: Risbud, Sumedh <[email protected]> Co-authored-by: PhilippPlank <[email protected]> Co-authored-by: Marcus G K Williams <Marcus G K Williams [email protected]>
Issue Number:
Objective of pull request:
Pull request checklist
Your PR fulfills the following requirements:
pyb) passes locallypyb -E unit) or (python -m unittest) passes locallyPull request type
Please check your PR type:
Improved MNIST tutorial
What is the current behavior?
What is the new behavior?
Does this introduce a breaking change?
Supplemental information
N/A