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

Refactor synapse delay #1536

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

Refactor synapse delay #1536

wants to merge 9 commits into from

Conversation

arvoelke
Copy link
Contributor

@arvoelke arvoelke commented Jun 3, 2019

Motivation and context:
Fixes #938 by correcting the behaviour in #1535 when D != 0. This is accomplished by refactoring the delay in the synapse, such that y = Cx + Du is set at the beginning of the time-step and x = Ax + Bu is updated at the end of the time-step.

In #1535 both equations were being updated at the end of the time-step, which caused them to be applied in the wrong order in the case of D != 0. This implements the correct set of equations for a discrete-time LTI system, and is consistent with old behaviour when D = 0.

Compared to the current master branch, systems that have D != 0 and len(A) > 0 no longer have a two time-step delay.

Another improvement with this PR is the Simulator produces the same results as applying filt, which in turn are both consistent with scipy.signal.lfilter. This was not the case before (regardless of D).

More details can be found here:

Interactions with other PRs:
Rebased onto #1387 (relies on having the state of the synapse being a signal), which is in turn rebased onto #1535 (state-space simulation of synapses).

How has this been tested?
Added test from #938 (comment) which now passes.
Rest of testing is a work-in-progress.

How long should this take to review?

  • Lengthy (more than 150 lines changed or changes are complicated)

Where should a reviewer start?
Work-in-progress.

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:

Feedback would be helpful in determining what is worth doing.

hunse and others added 4 commits May 28, 2019 10:52
Imported the version from Scipy. This makes filters stable even
with small tau.
Instead of transfer function representation. State space
appears to be faster, and potentially more accurate too.
Synapse states now use signals, allowing their state to be stored
for example when pickling a Simulator.
@arvoelke arvoelke changed the base branch from master to pickleable-sim June 3, 2019 05:10
@arvoelke arvoelke mentioned this pull request Jun 5, 2019
5 tasks
@drasmuss drasmuss force-pushed the pickleable-sim branch 2 times, most recently from 86a3704 to d63d6e7 Compare June 20, 2019 14:21
@tbekolay tbekolay force-pushed the pickleable-sim branch 4 times, most recently from 3c748cd to 99f585e Compare July 9, 2019 22:55
@tbekolay tbekolay changed the base branch from pickleable-sim to master July 9, 2019 23:17
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.

Off-by-one in LinearFilter.General step
2 participants