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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tutorial about functions and tuning curves. #1129

Merged
merged 1 commit into from
Apr 27, 2017
Merged

Conversation

jgosmann
Copy link
Collaborator

@jgosmann jgosmann commented Jul 23, 2016

Description:
Adds a tutorial/example notebook.

Motivation and context:
Seemed useful to me to have.

Interactions with other PRs:
none

How has this been tested?
Ran all cells of the notebook.

Where should a reviewer start?
Read the notebook. 馃摉

How long should this take to review?

  • Average (neither quick nor lengthy)

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.

@Seanny123
Copy link
Contributor

Would it be a good idea to introduce the idea of the thresholding preset at the conclusion of this notebook?

@Seanny123 Seanny123 removed their assignment Jul 25, 2016
@jgosmann
Copy link
Collaborator Author

Yes! I wrote most of this quite a while ago (when there was no preset) and managed just now to look it over and finish it up.

@jgosmann
Copy link
Collaborator Author

jgosmann commented Aug 2, 2016

Added the ThresholdingEnsembles preset.

@jgosmann jgosmann removed their assignment Aug 2, 2016
"cell_type": "markdown",
"metadata": {},
"source": [
"Let as look at the tuning curves first."
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo. Should be "Let us"

@Seanny123
Copy link
Contributor

Other than the typo, this LGTM.

@tbekolay
Copy link
Member

tbekolay commented Aug 9, 2016

I will review this shortly, but one thing I noticed in the diff is that this notebook uses notebook version 4 while all the other notebooks we've kept as version 3 so far. The notebook version was changed in IPython 3.0, and can be read by IPython 2.4.1. so we would effectively be ending support for IPython 2.4.0 and below. 2.4.1 was released February 2015, and our tests already depend on jupyter rather than IPython. Additionally, if we wanted, we could spit out version 3 notebooks during our doc building process even if the stored notebooks are version 4. So, there are a few options... vote using the emoji responses!

  • 馃槅 Upgrade all our notebooks to v4 but generate v3 notebooks to be linked in the docs.
  • 馃帀 Upgrade all our notebooks to v4; if people have older versions of IPython they can download the Python script equivalent instead of the notebook.
  • 馃槙 Keep our notebooks as v3 (and convert the notebook in this PR to v3).

Feel free to propose other resolutions as well.

@jgosmann
Copy link
Collaborator Author

jgosmann commented Aug 9, 2016

How easy is it to generate v3 from v4?
Debian stable is still at IPython 2.3. :/

@tbekolay
Copy link
Member

tbekolay commented Aug 9, 2016

How easy is it to generate v3 from v4?

Pretty trivial: jupyter nbconvert notebook.ipynb --to notebook --nbformat 3 --output notebook-3.ipynb

And there would be some programmatic way to do the same.

@jgosmann
Copy link
Collaborator Author

jgosmann commented Aug 9, 2016

If we can add that to the clear outputs script, I'm leaning towards keeping them in v3 for now. (Gotta call that script anyways to clear the outputs.)

@tbekolay
Copy link
Member

Let's keep them v3 for now (path of least resistance) as this would be nice to have for #1150. I'll modify the clear outputs script as well.

@tbekolay
Copy link
Member

Started looking at this in detail... one issue is that it uses spa bits, which we will eventually be moving elsewhere. I think it's pretty clear that this example is a general one and doesn't rely on the spa bits, so is it OK if I change this to a simpler example that doesn't require SPA stuff?

In either case, since this will take some time to change, I'm going to leave this until after the 2.2.0 release, as it's already somewhat overdue.

@Seanny123
Copy link
Contributor

@tbekolay given that Nengo 2.2.0 has be released, is this ready to be merged?

@jgosmann
Copy link
Collaborator Author

Probably not? It seems references to SPA should be removed?

@jgosmann jgosmann assigned jgosmann and unassigned jgosmann Apr 18, 2017
@jgosmann jgosmann requested a review from Seanny123 April 18, 2017 20:57
@jgosmann
Copy link
Collaborator Author

I rewrote the tutorial to remove references to SPA. Please review it again.

Copy link
Contributor

@Seanny123 Seanny123 left a comment

Choose a reason for hiding this comment

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

Still looks good.

Copy link
Member

@tbekolay tbekolay left a comment

Choose a reason for hiding this comment

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

Added some squash commits to add this example to docs, among other things. Let me know if you're okay with these changes @jgosmann (in particular, feel free to revert any rewordings you disagree with in the notebook). I'll merge on your OK!

@jgosmann
Copy link
Collaborator Author

LGTM

@tbekolay tbekolay merged commit bf47e49 into master Apr 27, 2017
@tbekolay tbekolay deleted the assocmem-tutorial branch April 27, 2017 17:37
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

3 participants