Navigation Menu

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

Presets #1077

Merged
merged 1 commit into from Jul 28, 2016
Merged

Presets #1077

merged 1 commit into from Jul 28, 2016

Conversation

jgosmann
Copy link
Collaborator

@jgosmann jgosmann commented May 31, 2016

Description:
Adds a thresholding preset as discussed in #1058. Other people are welcome to add their presets.

Motivation and context:
See #1058.

How has this been tested?
The thresholding preset is being used in the spa-rat model @ikajic and me are working on. I also implemented a unit test.

How long should this take to review?
It's a small PR, shouldn't take long to review.

Where should a reviewer start?
nengo/presets.py, look into nengo/tests/test_presets.py for a usage example.

Types of changes:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that causes existing functionality to change)

Checklist:

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

Still to do:

  • Changelog entry.
  • Link/reference in the documenattion?

@jgosmann
Copy link
Collaborator Author

I added the WIP label because other people might want to add more presets?
But apart from that my thresholding preset is ready for review.

@Seanny123
Copy link
Contributor

What do people think about a DirectPreset for setting neuron_type=nengo.Direct() and n_neurons=1? I use this when debugging my network and when I need to add spa.State() populations for visualisation.

@jgosmann
Copy link
Collaborator Author

I think n_neurons is ignored/irrelevant when setting neuron_type=nengo.Direct(). I only realized this myself recently when reading through some parts of the builder. Maybe this should be better documented? Because it is not really obvious. Maybe also something to mention as part of #1119.

@jgosmann
Copy link
Collaborator Author

Added a changelog entry which should make this ready for review. It was decided at one of the dev meetings to go ahead with this PR. New PRs can be opened for additional presets.

Only remaining thing might be to add some general documentation? But not sure where.

@jgosmann
Copy link
Collaborator Author

Note that the Travis-CI test failure is unrelated to this PR, but I don't want to restart the job to keep the documentation for the corresponding issue #1011.

@tbekolay
Copy link
Member

Only remaining thing might be to add some general documentation? But not sure where.

Hm. I think probably we need a new section in the user guide between the modelling API and the networks for user-facing config system documentation and presets. I'd be OK with leaving this for a separate PR!

@tbekolay
Copy link
Member

This PR LGTM! I added a commit fixing some reST formatting and grammar. In our API docs, we use a default Python role of "object", so the :class: part isn't needed (Sphinx figures out that it's a class through introspection).

I also renamed the preset to ThresholdingEnsembles ... that can be reverted, but IMO thinking about model scripts, I feel like

with ThresholdingEnsembles(0.5):
    ens = nengo.Ensemble(50, 1)

gives more of a clue of what happens than

with ThresholdingPreset(0.5):
    ens = nengo.Ensemble(50, 1)

partly because the Preset part is not super important, but also because it's not clear from the name that it only affect ensembles and not connections etc. But again, that's my opinion, the second reviewer can arbitrate if needed!

@Seanny123
Copy link
Contributor

This still LGTM.

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