Skip to content

Improve mnist tutorial#147

Merged
srrisbud merged 13 commits intolava-nc:mainfrom
srrisbud:improve_mnist_tutorial
Feb 10, 2022
Merged

Improve mnist tutorial#147
srrisbud merged 13 commits intolava-nc:mainfrom
srrisbud:improve_mnist_tutorial

Conversation

@srrisbud
Copy link
Copy Markdown
Contributor

@srrisbud srrisbud commented Dec 5, 2021

Issue Number:

Objective of pull request:

Pull request checklist

Your PR fulfills the following requirements:

  • Issue created that explains the change and why it's needed
  • Tests are part of the PR (for bug fixes / features)
  • Docs reviewed and added / updated if needed (for bug fixes / features)
  • PR conforms to Coding Conventions
  • PR applies BSD 3-clause or LGPL2.1+ Licenses to all code files
  • Lint (pyb) passes locally
  • Build tests (pyb -E unit) or (python -m unittest) passes locally

Pull request type

Please check your PR type:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation changes
  • Other (please describe):
    Improved MNIST tutorial

What is the current behavior?

  • MNIST tutorial does not show any meaningful classification.

What is the new behavior?

  • Now MNIST tutorial shows meaningful classification.

Does this introduce a breaking change?

  • Yes
  • No

Supplemental information

N/A

    - 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]>
@srrisbud srrisbud added 1-feature New feature request 0-needs-review For all new issues area: tutorials Issues with something in lava/tutorials labels Dec 5, 2021
@srrisbud srrisbud self-assigned this Dec 5, 2021
@srrisbud srrisbud linked an issue Dec 5, 2021 that may be closed by this pull request
8 tasks
Copy link
Copy Markdown
Contributor

@awintel awintel left a comment

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@phstratmann phstratmann left a comment

Choose a reason for hiding this comment

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

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.

@srrisbud
Copy link
Copy Markdown
Contributor Author

srrisbud commented Feb 6, 2022

@awintel, @phstratmann I have addressed your comments and pushed the changed version. Please re-review.

Copy link
Copy Markdown
Contributor

@awintel awintel left a comment

Choose a reason for hiding this comment

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

Looks fine now. But the whole story line should probably refreshed now that lava-dl exists at the end of the quarter.

@awintel
Copy link
Copy Markdown
Contributor

awintel commented Feb 7, 2022

Check this new issue as well before wrapping up this PR:
#166 (comment)

Copy link
Copy Markdown
Contributor

@phstratmann phstratmann left a comment

Choose a reason for hiding this comment

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

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]>
@srrisbud srrisbud requested a review from phstratmann February 9, 2022 13:00
Copy link
Copy Markdown
Contributor

@phstratmann phstratmann left a comment

Choose a reason for hiding this comment

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

Great work, thanks!

@srrisbud srrisbud merged commit 2a5ab6f into lava-nc:main Feb 10, 2022
mgkwill pushed a commit to mgkwill/lava that referenced this pull request Feb 15, 2022
* 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]>
joyeshmishra pushed a commit that referenced this pull request Feb 25, 2022
* 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]>
@srrisbud srrisbud deleted the improve_mnist_tutorial branch July 17, 2022 15:04
monkin77 pushed a commit to monkin77/thesis-lava that referenced this pull request Jul 12, 2024
* 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]>
monkin77 pushed a commit to monkin77/thesis-lava that referenced this pull request Jul 12, 2024
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

0-needs-review For all new issues 1-feature New feature request area: tutorials Issues with something in lava/tutorials

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MNIST tutorial needs improvement

4 participants