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

TernaryLIF and refactoring of LIF to inherit from AbstractLIF #151

Merged
merged 5 commits into from
Jan 24, 2022

Conversation

srrisbud
Copy link
Contributor

@srrisbud srrisbud commented Dec 8, 2021

Signed-off-by: Risbud, Sumedh [email protected]

Issue Number: #150

Objective of pull request: To merge TernaryLIF behaviour along with related refactoring of the LIF Process

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):

What is the current behavior?

  • There is no TernaryLIF

What is the new behavior?

  • There is a TernaryLIF, which spikes +1 or -1 depending upon which of the upper or lower thresholds was crossed.

Does this introduce a breaking change?

  • Yes
  • No

Supplemental information

  • The development required refactoring the existing LIF Process to carve out an AbstractLIF Process.
  • Both LIF and TernaryLIF inherit from AbstractLIF
  • This organization can be followed for organizing lava/proc library in general

@bamsumit
Copy link
Contributor

bamsumit commented Dec 9, 2021

Also, the type hints are missing. This is not fully consistent across all the repo, but we would want to incorporate it to reduce technical debt later.

pip install flake8-annotations
cd src/lava/proc/lif/
flake8 --ignore=ANN101,W503,E501 .

Copy link
Contributor

@GaboFGuerra GaboFGuerra left a comment

Choose a reason for hiding this comment

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

Thanks for the implementation! For now, I reviewed processes and processes tests. Will come back tomorrow to review models and models tests but posting now in case is useful.

src/lava/proc/lif/process.py Show resolved Hide resolved
src/lava/proc/lif/process.py Show resolved Hide resolved
src/lava/proc/lif/process.py Show resolved Hide resolved
src/lava/proc/lif/process.py Show resolved Hide resolved
src/lava/proc/lif/process.py Show resolved Hide resolved
src/lava/proc/lif/process.py Outdated Show resolved Hide resolved
src/lava/proc/lif/process.py Show resolved Hide resolved
tests/lava/proc/lif/test_process.py Show resolved Hide resolved
tests/lava/proc/lif/test_process.py Show resolved Hide resolved
tests/lava/proc/lif/test_process.py Show resolved Hide resolved
src/lava/proc/lif/models.py Outdated Show resolved Hide resolved
src/lava/proc/lif/models.py Outdated Show resolved Hide resolved
src/lava/proc/lif/models.py Show resolved Hide resolved
src/lava/proc/lif/models.py Outdated Show resolved Hide resolved
src/lava/proc/lif/models.py Outdated Show resolved Hide resolved
src/lava/proc/lif/models.py Show resolved Hide resolved
src/lava/proc/lif/models.py Outdated Show resolved Hide resolved
src/lava/proc/lif/models.py Outdated Show resolved Hide resolved
src/lava/proc/lif/models.py Outdated Show resolved Hide resolved
tests/lava/proc/lif/test_models.py Show resolved Hide resolved
Copy link
Contributor

@drager-intel drager-intel left a comment

Choose a reason for hiding this comment

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

Overall, I think this looks good.

In the next refactor, if we keep this Abstract parent class/inheritance style structure, I would take bias and bias_exp vars out of the AbstractLIF Process and make them part of a fixed-point mix-in for a child LIF class.

I would also allow for heterogeneous du, dv, v_th, etc within a LIF Process, in anticipation of the Loihi2 compiler.

But, we have some decisions to make about the organization about neuron Procs before then and I think these 2 items can wait a bit.

src/lava/proc/lif/models.py Show resolved Hide resolved
src/lava/proc/lif/models.py Show resolved Hide resolved
src/lava/proc/lif/process.py Show resolved Hide resolved
src/lava/proc/lif/process.py Show resolved Hide resolved
Copy link
Contributor

@GaboFGuerra GaboFGuerra left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for addressing some of the comments.

PS: You might add new issues for the comments were the discussion was inconclusive so that we don't forget about it ;)

@GaboFGuerra GaboFGuerra merged commit dab87b5 into lava-nc:main Jan 24, 2022
monkin77 pushed a commit to monkin77/thesis-lava that referenced this pull request Jul 12, 2024
…c#151)

* TernaryLIF and refactoring of LIF to inherit from AbstractLIF

Signed-off-by: Risbud, Sumedh <[email protected]>

* Minor changes post @bamsumit review

Signed-off-by: Risbud, Sumedh <[email protected]>

* Changes after @GaboFGuerra's code review

Signed-off-by: Risbud, Sumedh <[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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LCA/QP optimization modules need a TernaryLIF neuron model, which spikes +/-1 spikes with two thresholds
4 participants