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

Add some advanced features documentation. #1130

Merged
merged 1 commit into from
May 12, 2017
Merged

Add some advanced features documentation. #1130

merged 1 commit into from
May 12, 2017

Conversation

jgosmann
Copy link
Collaborator

@jgosmann jgosmann commented Jul 23, 2016

Description:
Extends the documentation with some advanced topics. In particular:

  • The Nengo RC file and available settings.
  • How to improve the performance.

Motivation and context:
The performance documentation is probably useful for new users who start to build larger models. Also it provides documentation for how to activate/deactivate the operator graph optimizer which is important because it provides a trade off between build time and simulation time.

In this documentation I had to reference the Nengo RC files which prompted me to add the missing documentation for that file and the settings too.

This PR closes #1119, addresses #1017.

Interactions with other PRs:
This adds documentation for the yet to be merged PR #1035. This assumes that the optimizer is active by default, but I could see this being changed. In that case this PR needs to be updated.

How has this been tested?
Build the docs and skimmed over it and tested cross-reference links. Should probably get a more careful read, preferably by a native speaker.

Where should a reviewer start?
Build the docs and start reading the "Advanced Topics" section.

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.

Still to do:

@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

build of a model will still take about the same time (technically a bit longer,
because the computed decoders will be stored), but all subsequent builds of the
same model can just load the stored decoders and will be faster. This will also
work to some degree if changes to the model are made, but of course only for
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove of course. It's kind of condescending. Maybe people don't know what caching is in this context.

@Seanny123
Copy link
Contributor

Left some inline comments to fix some phrasing.

@jgosmann
Copy link
Collaborator Author

I set this to "needs second review". Those changes can be easily made by whoever merges this if I don't get to it beforehand.

@mgserafi mgserafi self-assigned this Aug 12, 2016
@mgserafi
Copy link
Contributor

Made minor changes to the phrasing. Added @Seanny123's suggested changes.

@tbekolay
Copy link
Member

I also took a look at this, only partly in order to get my review in for the week ;)

Docs look great, really nice addition, and nice to have an advanced features section for more esoteric information 👍 I pushed a commit with some slight rewordings. I also added the WIP label because this is blocked by #1035.

One thing that I would like to do (but am not sure how to do best) is not duplicate the nengorc information. Right now, that information is nicely laid out in the new nengorc.rst doc, but similar (yet not the same) documentation exists in nengo-data/nengorc, and also in nengo/rc.py. Ideally it would only be in one place, so the question is where. I think I would prefer it to be in the new doc that's in this PR, and then we should link that that document (somehow) in rc.py, and remove it from nengorc.

One thing that matplotlib does is link to and include a copy of a sample rc file in there, which we could also do with the include directive. I think it'd be especially nice if the nengorc included there is barebones instead of the super verbose version that matplotlib has (since the .rst file will always be able to render a nicer version of the docs).

@jgosmann
Copy link
Collaborator Author

I think it'd be especially nice if the nengorc included there is barebones instead of the super verbose version that matplotlib has

I think it is nice to have a verbose sample rc file because then I can edit it and have all the required information right there without the need to navigate to the documentation.

@tbekolay
Copy link
Member

'kay, then why don't we include the verbose sample in the docs rather than duplicating the information?

@jgosmann
Copy link
Collaborator Author

Sure, but the sample file will need some updating (probably should've been updated in the first place).

@tbekolay tbekolay self-assigned this Aug 23, 2016
@tbekolay
Copy link
Member

Pushed a commit updating the sample file and rc.py so that there's only a tiny bit of duplication of information. In the generated output, the RC settings docs look pretty good to me now (the performance doc already looked good).

If you want to see the generated docs, I can send put the HTML files up somewhere, or you can try building the docs yourself. I do python setup.py build_sphinx; the only oddity is that you currently have to downgrade to ipython[all]==2.4.1 as the doc building hasn't been updated; I was working on updating it in the nbsphinx branch but that hit a few issues I haven't solved yet.

@jgosmann
Copy link
Collaborator Author

python setup.py build_sphinx

error: invalid command 'build_sphinx'

@jgosmann
Copy link
Collaborator Author

(I didn't have the requirement installed, but the error message is not really helpful in that case.)

@tbekolay
Copy link
Member

(I didn't have the requirement installed, but the error message is not really helpful in that case.)

Yeah... we could add something to setup.py to detect that but it doesn't really seem worth the additional code as probably three people in the world have tried to build the docs.

@jgosmann
Copy link
Collaborator Author

jgosmann commented Aug 23, 2016

To dos:

  • Mention that the optimizer can do certain optimizations only if SciPy is installed.
  • Add configuration options for optimizer to sample rc file.

@tbekolay
Copy link
Member

Add configuration options for optimizer to sample rc file.

I did in my commit, I think... at least, I added the ones that were in the nengorc.rst doc.

@jgosmann
Copy link
Collaborator Author

Seem to be missing to me ... (or maybe I forgot to add those in the first place)

@jgosmann
Copy link
Collaborator Author

The [OpMergeOptimizer] section heading is missing (that was what I was searching for).

@tbekolay
Copy link
Member

I added two options, see https://github.com/nengo/nengo/pull/1130/files#diff-e6c6a36ed8df7b1bd3ac3887920554ccR60, not sure if there are more...

@tbekolay
Copy link
Member

Ah, right you are! Should probably be op_merge_optimizer to match the other sections, but yeah, I missed that.

@jgosmann
Copy link
Collaborator Author

Should probably be op_merge_optimizer to match the other sections, but yeah, I missed that.

It needs to match the class name with the current implementation.

@jgosmann
Copy link
Collaborator Author

Fixed those things.

@tbekolay tbekolay removed their assignment Oct 18, 2016
@tbekolay
Copy link
Member

As per the discussion at the dev meeting, when this is merged, it should close #340.

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.

This still looks good to me. Special thanks to @mgserafi for the fixes I pointed out and really should have fixed myself.

@jgosmann jgosmann self-assigned this Apr 12, 2017
@jgosmann jgosmann removed their assignment Apr 18, 2017
@jgosmann
Copy link
Collaborator Author

Added commits to update this to match what has actually been merged to master.

@Seanny123
Copy link
Contributor

This is still good. The flake8 TravisCI errors should be fixed by bringing this branch up to date and I'm sure whoever resolves the conflicts will fix CHANGES.rst to refer to the right Nengo version.

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.

Pushed a fixup with a few final touchups. I'll merge on @jgosmann's OK!

@jgosmann
Copy link
Collaborator Author

LGTM 🍰

* Methods to improve the performance.
* Nengo RC file and documentation of possible settings.

  This mostly involves updating the docstrings in nengo/rc.py and
  including them in the documentation.

Also included an intersphinx mapping to the Python 3 docs so that
we can reference Python objects easily.
@tbekolay tbekolay merged commit 76939b0 into master May 12, 2017
@tbekolay tbekolay deleted the doc-performance branch May 12, 2017 20:28
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.

Document how to get the most speed out of Nengo
5 participants