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

Copy the current Microcuircit documentation for use with Readthedocs #1526

Merged
merged 28 commits into from
May 19, 2020

Conversation

steffengraber
Copy link
Contributor

@steffengraber steffengraber commented Apr 21, 2020

This PR fix issue #1524 and publish the updated microcircuit documentation on Read the Docs and for local use with sphinx.

fixes #1524
fixes #1511

@sarakonradi sarakonradi mentioned this pull request Apr 21, 2020
24 tasks
@heplesser heplesser added this to the NEST 3.0 milestone Apr 22, 2020
@heplesser heplesser added I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Enhancement New functionality, model or documentation labels Apr 22, 2020
Copy link
Contributor

@jhnnsnk jhnnsnk left a comment

Choose a reason for hiding this comment

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

Thanks, @steffengraber, for getting this started. As far as I see, the current PR is just about displaying the README in Readthedocs, but it should also be seen in the context of #1511 by @sarakonradi . Here are my comments:

  1. Currently the panel to the left provides links to the sections of the README which is unnecessary. Here we could consider to get links to the individual .py files of the model, or alternatively, just one link to a landing page of the model (for the latter see Link .py files in PyNEST microcircuit #1511) which then contains the links to the files. The files are already converted to .html (they appear in auto_examples/Potjans2014 when building the documentation), but they are not linked, yet, as far as I see.

  2. Some references in the README are not rendered nicely in Readthedocs and I would suggest to directly change the README itself for consistency with GitHub.

    1. The "from" in "network sketch from²" could be removed.
    2. "Funding for¹" could be changed to "Funding for the study by Potjans and Diesmann¹".
  3. How this model is listed among the PyNEST examples could be changed from "PyNEST microcircuit" to "Microcircuit model (based on Potjans and Diesmann, 2014)" to be more specific.

@steffengraber
Copy link
Contributor Author

@sarakonradi Thanks for your help!

@sarakonradi
Copy link
Contributor

@jhnnsnk Thanks for your detailed feedback. I updated the references and changed the link title, as you suggested in 2. and 3. Please check!

Currently the panel to the left provides links to the sections of the README which is unnecessary. Here we could consider to get links to the individual .py files of the model, or alternatively, just one link to a landing page of the model (for the latter see #1511) which then contains the links to the files. The files are already converted to .html (they appear in auto_examples/Potjans2014 when building the documentation), but they are not linked, yet, as far as I see

We haven't figured out a proper solution for that yet (see my latest comment in #1511). How I understood it in the VC earlier this week is that #1511 is not part of the NEST 3.0 milestone and that such link enhancements will be examined after the release.

So our main priority for this PR, I believe, should be to publish the new README on Read the Docs. We can improve the links at a later point in time. What do you think?

@sarakonradi
Copy link
Contributor

@jhnnsnk @steffengraber I created a new landing page and added the absolute link for reference_data, as discussed offline. The file links in the landing page still need to be updated and the README image fixed. @steffengraber Could you please have a look?

@steffengraber
Copy link
Contributor Author

@sarakonradi I fixed the links.

@sarakonradi
Copy link
Contributor

@steffengraber Thanks! The links work now.

Could you please check the images too? This is how they render at the moment:

Screenshot from 2020-04-23 12-48-00 (1)

@steffengraber
Copy link
Contributor Author

@sarakonradi @jhnnsnk The problem with the 3 pictures is solved.

@sarakonradi
Copy link
Contributor

@steffengraber Thanks!

Copy link
Contributor

@jhnnsnk jhnnsnk left a comment

Choose a reason for hiding this comment

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

Thank you so much, @steffengraber and @sarakonradi, for your work. This landing page is great!
I would only like to ask for some small changes:

  1. Please add the reference to Potjans and Diesmann as in the README.
  2. Directly after the first line, I suggest to write something like "Here you can inspect all files belonging to this example:"
  3. Then I'd remove the headline "File structure" and add as first bullet point something like "README: documentation of this microcircuit model implementation and its usage" with the "README" being clickable

@sarakonradi
Copy link
Contributor

@jhnnsnk I just updated the landing page. Please check!

@jhnnsnk
Copy link
Contributor

jhnnsnk commented Apr 23, 2020

@sarakonradi Looks great to me now! No further comments from my side (apart from this blank line Travis complains about).

@steffengraber
Copy link
Contributor Author

@jhnnsnk Travis is happy now.

@terhorstd Please review.

Copy link
Contributor

@jhnnsnk jhnnsnk left a comment

Choose a reason for hiding this comment

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

Thank you! 👍

Copy link
Contributor

@terhorstd terhorstd left a comment

Choose a reason for hiding this comment

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

Thanks for the work!
I found only one minor inconsistency when copying files, but that should be easy to solve.

doc/examples/pynest_microcircuit_link.rst Show resolved Hide resolved
doc/conf.py Outdated Show resolved Hide resolved
Copy link
Contributor

@terhorstd terhorstd left a comment

Choose a reason for hiding this comment

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

comments have been addressed. Thanks for the nice PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Enhancement New functionality, model or documentation
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Publish updated microcircuit documentation on Read the Docs Link .py files in PyNEST microcircuit
5 participants