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

Cache probe data on access. #1175

Merged
merged 1 commit into from
Sep 23, 2016
Merged

Cache probe data on access. #1175

merged 1 commit into from
Sep 23, 2016

Conversation

jgosmann
Copy link
Collaborator

@jgosmann jgosmann commented Sep 21, 2016

Motivation and context:
Repeated access to probes is slow (see #1076). This caches the read-only NumPy array to make it faster.

How has this been tested?
Ran this script adopted from @arvoelke in #1076 on master and this branch:

from __future__ import print_function

import time
import nengo

with nengo.Network() as model:
    x = nengo.Ensemble(1000, 1)
    p_neurons = nengo.Probe(x.neurons)

with nengo.Simulator(model) as sim:
    sim.run(10)

# Repeated probe accesses
start = time.time()
for m in range(x.n_neurons):
    sim.data[p_neurons][0, m]
print(time.time() - start)

# Only one probe access
start = time.time()
u = sim.data[p_neurons][0, :]
for m in range(x.n_neurons):
    u[m]
print(time.time() - start)

The repeated access block took 17s on master vs. 0.017s on this branch. The second single access block was reduced to 0.00017s from 0.017s. (I don't know what's up with all the 17s ... 👻 )

How long should this take to review?

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

Where should a reviewer start?
The usual place: “Files changed”
Note that the cache does not need to use weak references because the raw dict in the ProbeDict class doesn't use weak references either. Thus, as long as the ProbeDict instance is around, there will always be a strong reference to each key.

Types of changes:

  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have read the CONTRIBUTING.rst document.
  • I have included a changelog entry.
  • All new and existing tests passed.
  • I have added tests to cover my changes.
  • [na] I have updated the documentation accordingly.

@mention-bot
Copy link

@jgosmann, thanks for your PR! By analyzing the annotation information on this pull request, we identified @tbekolay to be a potential reviewer

@Seanny123
Copy link
Contributor

LGTM.

@tbekolay
Copy link
Member

Awesome, that's a huge speedup! 🌈

Looking through the diff, will this give incorrect results in the following case?

with nengo.Simulator(net) as sim:
    sim.run(0.1)
    d = sim.data[probe]
    sim.run(0.1)
    assert d.shape != sim.data[probe].shape

If so, then I can see two possible solutions... either invalidate the cache in Simulator.step, or only enable the cache once we've closed the sim. Invalidating seems better, but since we call step a bunch, it might slow things down slightly...

@jgosmann
Copy link
Collaborator Author

Good catch!
I pushed a test and fix for this. I opted for a slightly different solution where I check the length of the array and list in the probe dict. That's is still a quick operation on access, allows to do the caching before sim.close() and doesn't require any additional code in the step function.

@tbekolay tbekolay self-assigned this Sep 22, 2016
@tbekolay
Copy link
Member

I pushed a test and fix for this. I opted for a slightly different solution where I check the length of the array and list in the probe dict.

Nice, that's a better fix for sure! I'll take a closer look now, likely merge shortly.

p = nengo.Probe(ens)

dt = 0.001
with nengo.Simulator(model, dt=dt) as sim:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this should be RefSimulator. I've fixed it locally.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I though of that when writing the function signature, but then I forgot when writing the actual code. 😨

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, force of habit 😉 🌈

This makes repeated access faster.

Fixes #1076.
@tbekolay tbekolay merged commit ceaf387 into master Sep 23, 2016
@tbekolay tbekolay deleted the cache-probe-data branch September 23, 2016 20:34
@tbekolay tbekolay removed their assignment Oct 16, 2017
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

4 participants