Three Factor Learning #400
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…into dev/learning_rc_0.5
…into dev/learning_rc_0.5
tim-shea
approved these changes
Oct 25, 2022
…-public into dev/learning_three_factor
…/dev/learning_three_factor
Bala/dev/learning three factor
awintel
approved these changes
Oct 25, 2022
There was a problem hiding this comment.
Looks better but still there are a couple of "cosmetic" things you should improve:
(Will approve nonetheless to not hold up the merge once you've addressed these)
- Please check all your headlines again. There are still several examples with random lower/upper case spelling like: "Initialize Network Processes", "Connect network Processes", "Tag Dynamics for Post-synaptic Neurons", "Reward Trace for Post-synaptic Neurons"
- "...how the improved Lava learning API..." -> "improved" is out of context. "Improved" over what?
- There is no hyphen between Loihi and 2. Sometimes you also say Loihi2 without white space.
- The intro paragraph doesn't seem accurate or maybe is outdated with the code: "... first define interface for RSTDP". I don't see an interface definition. You only instantiate a pre-build rule and build a network to use it. Only in the very end you define a custom rule.
- I would pull the note about Loihi 2 unavailability into an emphasis box. This way it's clearer that this is temporary and also reduces the chance that we forget to update the text once the code changes.
- Wrong string before the definition of your post neurons: "# Pre-synaptic neuron parameters"
- Although I now understand why there are two post neurons based on our discussion. The reader of this tutorial has no idea at this point of the tutorial why you chose two. Please motivate why there are two not just state that there are two. It's because you want to demonstrate something specific in the end.
- Could you align the x axis of the spike and trace plots?
- As pointed out in my last review, I feel the tutorial would benefit from at least a minimal explanation/discussion of the final results. You show one figure after the other but what should the reader take note of?
- Is this correct: "Frémaux"? Looks messed up.
- Is there now way around exposing the name LoihiUCLearningRule at the user level that should be agnostic of HW details? This is unfortunate...
gkarray
approved these changes
Oct 26, 2022
There was a problem hiding this comment.
Looks good overall.
Left comments on minor things, but will approve nonetheless to not hold up the release. If these can be addressed now, good, otherwise, they can be left for a future release.
One final comment :
- I think the folder
src/lava/proc/learning_rulesshouldn't go in theproclibrary at all. It does not contain any Process definition. This folder should probably instead be moved tosrc/lava/magma/core/learning/learning_rules
|
In the latest changes I addressed some of the comments above:
|
|
Regarding the correct place for the high level learning rules such as STDP. Strictly speaking, @gkarray is correct, they are not Processes, so they should be somewhere else. But in my opinion, they should stay in the process library as this is the place where a user is looking for commonly used models such as LIF. Our high level implementations of learning rules serve the same purpose. Additionally, I would like to avoid imports from deep inside lava for common use-cases. |
monkin77
pushed a commit
to monkin77/thesis-lava
that referenced
this pull request
Jul 12, 2024
* inheritance from base class * inheritance from base connection * floating point model * minor cleanup * added neuron base class * separate models from process files * rm init file * inherit from Neuron and Connection * merged LearningDense into Dense * release candidate 0.5 * beginning last clean * clean in progress * updated tutorial * changes according to checklist made. missing use of np.where refactor * changed evaluate_trace to use boolean indexing instead of np.where * removed BITS_ * lint and poetry update * moved tutorial to in_depth * Licenses * lint tests * poetry update * docstrings for connection and learning rule * fixing typo * minor optimization for floating point model * added learning related unit tests * added other learning related unit tests * refactored docstrings * merged Loihi1 and 2 LearningRules and added seed for RNGs * added neuon base class and three fac learning rule * minor changes * Adding R-STDP wrapper and tutorial skeleton * Adding decay to tag * adding LearningLIF process and update tutorial * adding LearningLIF process model * Modifying reward trace based on graded input * Update LearningLIF to include graded input spikes * Updated tutorial * Updating unit tests * Fixing errors * Editing tutorial text * modified tutorial and fixing dangling ports * adding tutorial_helper func * adding some plots to tutorial * Merge origin/main to origin/dev/learning_three_factor * added graded in ports to LearningDense and fixing tutorial plots * changes structure * added minor changes to stdo tutorials * fixing np.logical_and/or problems * tutorial polishing * weights plot fixed * fixing graded input ports in Dense * fixing lint errors * fixing LearningLIF run_spk * tutorial text changes * fixing lint errors * lint issues * annoying white spaces fixed * removing enable_learning flag * adding plot explanations * fixing rstdp wrapper * fix the stdp tests. minor update to structure * minor change * Saving tutorial text updates to new file. Avoiding merge conflict. * add y1 ports and major changes to the learning rules * minor changes * changing plastic to learning and wrapper name * fixed bugs and inconsistencies, moved RSTDP LIF to tutorial * including float to literal str * fixing lint errors * modifying tutorial * fixing lif tests * removing LearningLIF test * fixing tutorial image * changing tutorial name * moving image to lava-docs * fixing some of the comments from AW * renames run_spk in connection, HCand UC to 2F resp 3F, docstrings ands tests * fetch pyproject and lock from main * fixing tutorials plots, changing docstrings * adding tutorial text Co-authored-by: Philipp <philipp.weidel@intel.com> Co-authored-by: gkarray <ghassen.karray@intel.ch> Co-authored-by: drager-intel <danielle.rager@intel.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue Number:
Objective of pull request:
Implements the necessary changes to LAVA to support three factor learning rules. Contains a tutorial that implements a reward-modulated STDP learning rule. This PR also proposes structural changes to the Dense and LIF process and process models to include base classes specific to learning.
Pull request checklist
Your PR fulfills the following requirements:
flakeheaven lint src/lava tests/) and (bandit -r src/lava/.) pass locallypytest) passes locallyPull request type
Please check your PR type:
What is the current behavior?
What is the new behavior?
Does this introduce a breaking change?
Supplemental information