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.

Sign up for GitHub

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

Jump to bottom

Conv Process Implementation #73

Merged
merged 16 commits into from
Nov 24, 2021
Merged

Conv Process Implementation #73

merged 16 commits into from
Nov 24, 2021

Conversation

bamsumit
Copy link
Contributor

@bamsumit bamsumit commented Nov 18, 2021 •
edited
Loading

Implementation of conv process and process models.

awintel
awintel requested changes Nov 18, 2021
View reviewed changes
Copy link
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 good. Just some minor comments.

tests/lava/proc/conv/test_models.py Outdated Show resolved Hide resolved
src/lava/proc/conv/process.py Outdated Show resolved Hide resolved
src/lava/proc/conv/process.py Outdated Show resolved Hide resolved
src/lava/proc/conv/process.py Outdated Show resolved Hide resolved
src/lava/proc/conv/utils.py Show resolved Hide resolved
src/lava/proc/conv/models.py Show resolved Hide resolved
src/lava/proc/conv/models.py Outdated Show resolved Hide resolved
src/lava/proc/conv/models.py Show resolved Hide resolved
tests/lava/proc/conv/test_models.py Outdated Show resolved Hide resolved
tests/lava/proc/conv/test_models.py Outdated Show resolved Hide resolved
srrisbud
srrisbud requested changes Nov 19, 2021
View reviewed changes
Copy link
Contributor

@srrisbud srrisbud left a comment

Choose a reason for hiding this comment

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

There are a few changes needed. Especially the ReceiveProcess has its ProcessModels flipped.

src/lava/proc/conv/process.py Outdated Show resolved Hide resolved
src/lava/proc/conv/utils.py Show resolved Hide resolved
src/lava/proc/conv/utils.py Outdated Show resolved Hide resolved
src/lava/proc/conv/utils.py Outdated Show resolved Hide resolved
src/lava/proc/io/sink.py Outdated Show resolved Hide resolved
src/lava/proc/io/sink.py Outdated Show resolved Hide resolved
src/lava/proc/io/sink.py Outdated Show resolved Hide resolved
src/lava/proc/io/source.py Outdated Show resolved Hide resolved
src/lava/proc/io/source.py Outdated Show resolved Hide resolved
tests/lava/proc/conv/test_models.py Show resolved Hide resolved
awintel
awintel reviewed Nov 19, 2021
View reviewed changes
Copy link
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.

Just some additional comments

drager-intel
drager-intel requested changes Nov 19, 2021
View reviewed changes
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.

My only 2 comments are surrounding unit tests. Can we add some unit tests for the Conv Connection behavior that don't rely on Torch?

tests/lava/proc/conv/test_process.py Outdated Show resolved Hide resolved
tests/lava/proc/conv/test_utils.py Show resolved Hide resolved
mathisrichter
mathisrichter requested changes Nov 19, 2021
View reviewed changes
Copy link
Contributor

@mathisrichter mathisrichter left a comment

Choose a reason for hiding this comment

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

Looks great, Sumit!

  • Not sure whether this is a general convention but elsewhere we use type hints in class, method, and function signatures. Maybe add an issue for that and do it later.

src/lava/proc/conv/models.py Outdated Show resolved Hide resolved
src/lava/proc/conv/models.py Outdated Show resolved Hide resolved
src/lava/proc/conv/models.py Show resolved Hide resolved
src/lava/proc/conv/models.py Outdated Show resolved Hide resolved
src/lava/proc/conv/process.py Outdated Show resolved Hide resolved
tests/lava/proc/conv/test_models.py Outdated Show resolved Hide resolved
tests/lava/proc/conv/test_models.py Outdated Show resolved Hide resolved
tests/lava/proc/conv/test_models.py Show resolved Hide resolved
tests/lava/proc/conv/test_utils.py Show resolved Hide resolved
tests/lava/proc/io/test_source_sink.py Outdated Show resolved Hide resolved
@mgkwill
Copy link
Contributor

mgkwill commented Nov 20, 2021

Starting pull request

This is probably clear to some due to internal conversations but the title is not descriptive and I don't see an issue for this pull.

Can we please follow the process, I.E. open an issue that explains the change and have at least a minimal explanation of the work in the pull request?

@bamsumit bamsumit linked an issue Nov 20, 2021 that may be closed by this pull request
Conv process implementation #81
Closed
@mgkwill mgkwill changed the title Conv clean Conv Process Implementation Nov 21, 2021
@mgkwill mgkwill mentioned this pull request Nov 23, 2021
Closed
awintel
awintel approved these changes Nov 23, 2021
View reviewed changes
Copy link
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.

Nothing major except for missing indentation of function arguments that start in a new line. That makes code not very readable.
Could be fixed later but we should not leave it like that.

drager-intel
drager-intel approved these changes Nov 23, 2021
View reviewed changes
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.

Thanks for adding a known input-output unit test of conv behavior for users w/out Torch installed! Looks good!

@srrisbud srrisbud merged commit 89b21f7 into lava-nc:main Nov 24, 2021
@bamsumit bamsumit deleted the conv-clean branch July 22, 2022 13:38
monkin77 pushed a commit to monkin77/thesis-lava that referenced this pull request Jul 12, 2024
@bamsumit @srrisbud
Conv Process Implementation (lava-nc#73)
5f2e439 
* Conv process implementation with unittests.

Signed-off-by: bamsumit <[email protected]>

* updated requirements and test inits

Signed-off-by: bamsumit <[email protected]>

* tweak unittests

Signed-off-by: bamsumit <[email protected]>

* fix merge conflict

Signed-off-by: bamsumit <[email protected]>

* Fix to comments, fixed lif warnings, refactored source sink processes and their unittests

Signed-off-by: bamsumit <[email protected]>

* flesh source/sink nomenclature. Fix overflow/underflow bug. Resolved Sudmedh's comments

Signed-off-by: bamsumit <[email protected]>

* New ground truth based conv implementation test

* type hinting fix

Signed-off-by: bamsumit <[email protected]>

* removed unnecessary commented code

Signed-off-by: bamsumit <[email protected]>

* typehinting consistency fix: np.array -> np.ndarray

Signed-off-by: bamsumit <[email protected]>

* static seeds in unittest

Signed-off-by: bamsumit <[email protected]>

* fixes for clean init merge

Signed-off-by: bamsumit <[email protected]>

Co-authored-by: Risbud, Sumedh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drager-intel drager-intel drager-intel approved these changes

@awintel awintel awintel approved these changes

@srrisbud srrisbud srrisbud approved these changes

@mathisrichter mathisrichter mathisrichter approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Conv process implementation
6 participants
@bamsumit @mgkwill @mathisrichter @srrisbud @awintel @drager-intel