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

LearningRuleTypes have "delta" as first probeable #1682

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hunse
Copy link
Collaborator

@hunse hunse commented Jan 25, 2022

Motivation and context:

While best practice is to explicitly pass the attr when probing learning rules, it seems sensible to also have some consistent default. The logical choice is "delta", since all learning rules must have this.

Since this is a breaking change (albeit minor), we'll hold off until Nengo 4.0.

How long should this take to review?

  • Quick (less than 40 lines changed or changes are straightforward)

Types of changes:

  • Breaking change (fix or feature that causes existing functionality to change)

Checklist:

  • I have read the CONTRIBUTING.rst document.
  • I have updated the documentation accordingly.
  • I have included a changelog entry.
  • I have added tests to cover my changes.
  • I have run the test suite locally and all tests passed.

Still to do:

  • Decide if we want to build this into the base LearningRuleType or LearningRule itself, such that "delta" is inserted at the front automatically.

Also, will now raise a ValidationError if `obj.probeable` is empty
on a Probe, such as for a custom LearningRule with no probeables.
@hunse hunse added this to the nengo-4.0 milestone Jan 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

1 participant