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

Make op 'set' semantics more consistent #1596

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Make op 'set' semantics more consistent #1596

wants to merge 4 commits into from

Conversation

drasmuss
Copy link
Member

Motivation and context:

We organize the signals associated with an Operator according to whether the operator sets, increments, reads, or updates that signal. The difference between a "set" and an "increment" is that an increment applies some delta to the existing value of a signal, whereas a set overrides whatever the existing value was. In a couple places we had marked an operation that incremented a signal as a set instead.

In particular:

  • TimeUpdate.step. This op is implementing step += 1, so step should be an inc rather than a set.
  • SimNeurons.states. This one isn't as clear cut, since we don't really know how a neuron model might be using the state. However, I think we should assume that the neuron model will be reading
    the value of the state and applying some change (that's why it needs to track the state), rather than
    ignoring the current state value and setting it to some value. So this makes more sense as an inc than a set. It could be argued that this should be an update rather than an inc, but that would change the semantics of any signal probes that read these state values, so I went with inc.

In practice this change doesn't change the behaviour of the model at all (since sets and incs are scheduled consecutively, with no other ops in between, moving a signal from one to the other doesn't really matter). But this can matter in other backends (e.g. NengoDL) which are relying on the assumption that sets really are sets (i.e. do not depend on the current value of the signal), which is where I noticed this.

I also added tags to a couple ops that didn't have them that I noticed while looking at this.

Interactions with other PRs:

Based on #1591 (since that one has some general fixes required to get the CI passing).

How has this been tested?

All the tests pass without modification after this change, so there is no functional effect (as we'd expect).

How long should this take to review?

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

Types of changes:

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

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

This avoids creating any extra transform operations, rather
than always adding a transform=1 operation.

Update dev version to 3.1.0
Also add some tags to ops that didn't have them.
@tbekolay tbekolay force-pushed the transform-none branch 3 times, most recently from 0fedf35 to 6b1f292 Compare March 3, 2020 19:42
@tbekolay tbekolay changed the base branch from transform-none to master March 6, 2020 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant