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

Nonseq #245

Open
wants to merge 393 commits into
base: develop
Choose a base branch
from
Open

Nonseq #245

wants to merge 393 commits into from

Conversation

ssinhaleite
Copy link
Member

@ssinhaleite ssinhaleite commented Jul 10, 2024

Checklist before requesting a review

Unit tests

  • Tests for the changes have been added (for bug fixes/features)
  • All tests are passing.

Documentation

  • Add tutorial(s) about how to deploy a non-sequential model
  • Fix any inconsistencies with existing documentation (e.g. changed function parameters, deprecated methods, etc.)
  • Add graph that shows developers how different classes interact
  • Make sure in-code documentation is complete (e.g. missing or incomplete docstrings)
  • CHANGELOG.md: Make sure to list any breaking changes

Review

  • I have performed a self-review of my code
  • Address open suggestions from the review (typos etc)
  • Take care of all TODOs inside the code
  • What 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.

Copy link
Contributor

@bauerfe bauerfe left a 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

Copy link
Contributor

@bauerfe bauerfe left a 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.

Copy link
Contributor

@bauerfe bauerfe left a 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.

@ssinhaleite ssinhaleite marked this pull request as ready for review February 24, 2025 14:04
@ssinhaleite ssinhaleite requested a review from bauerfe February 24, 2025 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants