-
Notifications
You must be signed in to change notification settings - Fork 10
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
Nonseq #245
base: develop
Are you sure you want to change the base?
Nonseq #245
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a full review, but I will finish it for now so the suggestions become visible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still not complete, will add further comments over time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like there is still some work to do. In the interest of a speedy deployment, I will start applying changes myself. This means I won't be eligible as a (final) reviewer anymore.
@Willian-Girao , let's keep working on this based on the changes I suggested (and will continue suggesting). Whenever one issue has been addressed we can comment on it, as you have done previously. Ideally the other person would then have a brief look at it and resolve the conversation if everything is fine.
In the end there will be another person that will have to review all the changes, but ideally everything will be in place by then and the review will be quicker.
- Add `Addtributes` list to docstring of NIRtoDynapcnnNetworkGraph for better understanding - Rename `get_edges_list` property to `edges_list`
Updated my local black version to 24.10.0 as the CI
Remove "which should never happen" from them
DVS is an acronym, and on documentation/docstrings, it should be capitalized.
My local torch is 2.6.0 and tests are passing
Checklist before requesting a review
Unit tests
Documentation
Review
TODO
s inside the codeWhat kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
New feature, including extensive changes in code, documentation and unit tests.
What is the current behavior? (You can also link to an open issue here)
So far, only sequential models can be deployed on DynapCNN chips.
What is the new behavior (if this is a feature change)?
Allow non-sequential SNNs (e.g. residuals, recurrent, ...) to be deployed on DynapCNN chips.
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Potentially. Ideally for sequential SNNs there are no (or minimal) breaking changes, but this has to be assessed when reviewing this PR.
Other information:
The changes are quite extensive, so reviewing this will take some time.