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

enhance(tutorial6): net definition and link #157

Merged
merged 4 commits into from Dec 14, 2022

Conversation

gianfa
Copy link
Contributor

@gianfa gianfa commented Dec 7, 2022

  • make the Net class foward method single step
  • insert link to the tutorial 5 where cited

### Motivation
There are several points in this very useful tutorial for which I propose this ameliorative change.
For clarity I will call "v1" the current version of the Net definition and "v2" the version that is proposed in this PR.

  • An error launching "Run all". This is a shape incompatibility error in the loss calculation, because in v1 there is an internal loop in .forward, which thus adds an extra axes. Now it runs all cells with no errors.
  • .forward in v1 is apparently inconsistent with .forward in v2, being the former with an inner loop (so multi-step), while the latter without it (so single-step).
  • (am I overthinking?) Whether there was an intention behind defining the network with an inner loop to define a smaller dt, characteristic of an inner frequency higher than the outer frequency (receptors) is not very clear and it might be beneficial to make it explicit.

It is clear that the tutorial 5 example was being cited, but in this context it comes across as a bit inconsistent.
If I have misunderstood, please tell me how I should best understand the tutorial.

Thank you for the effort of creating this project :)

* make the Net class `foward` method single step
* insert link to the tutorial 5 where cited
@jeshraghian
Copy link
Owner

I appreciate you taking the time to help contribute!

I hit run all but did not face any errors. I think what may be happening is you're using the Net class [block 8] instead of the Sequential wrapper [block 9].

The Net Class in block 8 is just referring back to the previous way Tutorial 5 instantiated the network.
It is never actually used in this tutorial or assigned to a variable.

Instead, we assign the Sequential wrapper to net, and it does not use a for-loop. So a new loop must be defined.

Does this make sense?

@gianfa
Copy link
Contributor Author

gianfa commented Dec 11, 2022

Yes, you are completely right, sorry for the lack of clarity.

What I meant is just that if I decide to use the non-sequential definition of the network I can't execute all the cells.

This, IMHO is an unexplained inconsistency. Why should the forward step be multiple (loop) in one network and not be in another?
I am aware that everyone can handle it as they wish, but I wonder if it is appropriate from a design perspective.

For example, I think Sequential is purposely created single step, as it resides on an external loop that is always present in pytorch, by convention. If I include an internal loop in the network definition, I should do so thinking that this is additional to the external loop (case of internal trarining batch step for each external step), not just alternative implementation.

Maybe it is irrelevant to the creator of the tutorial :) , in which case you can close this PR, but if so I propose to consider defining a convention, since, especially in the spiking world, differences in dt in training (max frequency of the single neuron with respect to an "external" time flow) matter a lot and not having a standard can lead to conceptual error.
If I am still not very clear, please let me know.
Thank you.

@jeshraghian
Copy link
Owner

jeshraghian commented Dec 11, 2022

You're right; we definitely don't want any double loops!

When using sequential, we're stuck with an external loop. With the class, we can do either; I'd say it's just a coding style preference. The user is definitely not meant to double-loop the class; we want to ensure function is the same between the class and the sequential wrapper. I personally prefer an inner loop within the class to isolate the training loop from the time loop; its just easier to parse imo.

The class isn't meant to be used in the training loop at all. It's just there to identify what we're trying to build using nn.Sequential, using code the readers have already seen from the previous tutorial.

Let me run this past some of the discord members as a test crowd to see if the tutorial is unclear, and then decide if the code should be changed or just the description.

Appreciate the feedback though; I'll let you know here what I go with!

@jeshraghian
Copy link
Owner

I think your view is good & will lead to less confusion.
Let's make the change.

I'll push an update to the tutorial 6 static html documentation (as it doesn't automatically compile), update the description, and then merge the two shortly.

@jeshraghian jeshraghian merged commit 93fc25d into jeshraghian:master Dec 14, 2022
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.

None yet

2 participants