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

Sparsity netx pr #238

Merged
merged 11 commits into from
Oct 3, 2023
Merged

Sparsity netx pr #238

merged 11 commits into from
Oct 3, 2023

Conversation

Michaeljurado42
Copy link
Contributor

@Michaeljurado42 Michaeljurado42 commented Sep 18, 2023

Issue Number: #239

Objective of pull request: This code provides netX with the ability to interpret synapses as sparse types when converting SNNs from lava-dl to lava.

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 applys BSD 3-clause or LGPL2.1+ Licenses to all code files
  • Lint (flakeheaven lint src/lava tests/) and (bandit -r src/lava/.) pass locally
  • Build tests (pytest) 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?

What is the new behavior?

Does this introduce a breaking change?

  • Yes
  • No

Supplemental information

Copy link
Contributor

@bamsumit bamsumit left a comment

Choose a reason for hiding this comment

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

@Michaeljurado42 This looks good. Thanks for this one. Only one suggestion. Let's use sparse_fc_layer instead of sparsity_map

src/lava/lib/dl/netx/hdf5.py Outdated Show resolved Hide resolved
@bamsumit bamsumit marked this pull request as ready for review September 25, 2023 14:36
@Michaeljurado42
Copy link
Contributor Author

Michaeljurado42 commented Sep 30, 2023

Also, a final naming change which may be beneficial could be to rename the Dense proces defined in netx.blocks to FullyConnected or something. This would help us to not violate the naming convention in Lava which differentiates between dense and sparse layers. Similarly create_dense would become create_fully_connected. Your previous comment brought this to my attention.

Thank you for the feedback so far!

@bamsumit
Copy link
Contributor

bamsumit commented Oct 3, 2023

Also, a final naming change which may be beneficial could be to rename the Dense proces defined in netx.blocks to FullyConnected or something. This would help us to not violate the naming convention in Lava which differentiates between dense and sparse layers. Similarly create_dense would become create_fully_connected. Your previous comment brought this to my attention.

Thank you for the feedback so far!

On board with that. It would break a lot of things at this stage of this project though.

Copy link
Contributor

@bamsumit bamsumit left a comment

Choose a reason for hiding this comment

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

Thanks a lot @Michaeljurado42

@bamsumit bamsumit merged commit c5fcf7b into lava-nc:main Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants