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.

Sign up for GitHub

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

Jump to bottom

Made encoders probeable from ensembles #1167

Merged
merged 2 commits into from
Nov 3, 2016
Merged

Made encoders probeable from ensembles #1167

merged 2 commits into from
Nov 3, 2016

Conversation

bmorcos
Copy link
Contributor

@bmorcos bmorcos commented Sep 8, 2016 •
edited by Seanny123
Loading

Motivation and context:
Feature request from #1117

Interactions with other PRs:
Change also implemented in #553 commit 4a572c3

How has this been tested?
Updated test_probe.py and test_learning_rule.py

How long should this take to review?

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

Types of changes:

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

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.
  • All new and existing tests passed.

@mention-bot
Copy link

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

@jgosmann
Copy link
Collaborator

jgosmann commented Sep 8, 2016 •
edited
Loading

Thank you for the PR! I added a commit to rename the probed attribute to scaled_encoders because that is what is getting probed.

@bmorcos You need to sign Contributor Assignment Agreement (instructions are under that link) before we can merge this PR.

(This should also get a short changelog entry.)

@Seanny123
Copy link
Contributor

LGTM. Although the history is a bit messy, but I guess we'll leave that up to the maintainers to fix on merge?

@Seanny123
Copy link
Contributor

@tbekolay this has been ready to merge for a while. Any chance of getting it merged soon?

tbekolay
tbekolay reviewed Nov 2, 2016
View reviewed changes
nengo/tests/test_learning_rules.py
@@ -411,6 +412,8 @@ def test_voja_encoders(Simulator, nl_nodirect, rng, seed):
assert np.allclose(
sim.data[p_enc][tend, :n_change] / encoder_scale[:n_change],
learned_vector, atol=0.01)
# Check that encoders probed from ensemble euqal encoders probed from voja
Copy link
Member

@tbekolay tbekolay Nov 2, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Typo (euqal) -- will fix in merge, noting here so I don't forget

tbekolay
tbekolay reviewed Nov 3, 2016
View reviewed changes
nengo/tests/test_probe.py
ens = nengo.Ensemble(n_neurons=10, dimensions=2, radius=1.5)
p_enc = nengo.Probe(ens, 'scaled_encoders')

with nengo.Simulator(model) 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.

In the future, use the Simulator pytest fixture here rather than instantiating nengo.Simulator directly. You can see how this is done in other tests in this file, like test_dts. I'll fix this for now though :)

@bmorcos @tbekolay
Made encoders probeable from ensembles
e1b4601 
Encoders can still be probed via learning rule.
@tbekolay tbekolay merged commit e1b4601 into master Nov 3, 2016
@tbekolay tbekolay deleted the probe-encoders branch November 3, 2016 14:23
@tbekolay
Copy link
Member

tbekolay commented Nov 3, 2016

Merged, congrats to @bmorcos on your first merged PR! 😃 🌈

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tbekolay tbekolay tbekolay left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@bmorcos @mention-bot @jgosmann @Seanny123 @tbekolay