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

Fix and update model_details notebooks #2453

Merged
merged 20 commits into from
Dec 6, 2022

Conversation

jessica-mitchell
Copy link
Contributor

@jessica-mitchell jessica-mitchell commented Aug 19, 2022

This PR moves the model_details notebooks up to top level of hierarchy in htmldoc and fixes issues with rendering of the LaTeX math.

To do this the notebooks were converted to a reStructured text file and output as pngs,
A download link for each notebook was added using a zip file. A direct download of a notebook is not obviously possible with reStructuredText- (if one uses the :download: role with a .ipynb file, the raw notebook code opens).

See for example: https://nest-test.readthedocs.io/en/fix-notebooks-docs/model_details/HillTononiModels.html

Note:
The model_details folder is currently not indexed and not discoverable through the table of contents. They are linked in the respective models/ documentation.

The only notebook not changed is the aeif_models_implementation.ipynb because it does not execute properly. See issue #2454 .

@jessica-mitchell jessica-mitchell added S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation. I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) need reviewer Ready to be reviewed but no reviewer assigned labels Aug 19, 2022
@jessica-mitchell jessica-mitchell added this to In progress in Documentation via automation Aug 19, 2022
Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

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

@jessica-mitchell Thanks a lot for this PR! I see three issues with the current state of the PR

  1. The figures render extremely small in both Safari and Firefox on macOS.
  2. I would like to keep the notebooks as the primary documents, so that one can easily modify them if needed. RST-files and zip archives should then be automatically generated from the notebooks. Would this be feasible?
  3. Also, adding zip archives and generated figures to Git risks clogging the repo with binary files as these files evolve. Can we avoid it and just "make" those files on our documentation server?

@jessica-mitchell
Copy link
Contributor Author

@jessica-mitchell Thanks a lot for this PR! I see three issues with the current state of the PR

1. The figures render extremely small in both Safari and Firefox on macOS.

Sorry, this is because it is not up-to-date with master!

2. I would like to keep the notebooks as the primary documents, so that one can easily modify them if needed. RST-files and zip archives should then be automatically generated from the notebooks. Would this be feasible?

This is a fair point, but I had trouble getting the HTML rendered notebook to generate proper latex and the restructured text version worked easily. I will take another look to see how I can fix it using the notebook as source.

3. Also, adding zip archives and generated figures to Git risks clogging the repo with binary files as these files evolve. Can we avoid it and just "make" those files on our documentation server?

Making these files with sphinxgallery is possible, but it expects a python source. There are other ways I'm sure, which I can look into more.

@jessica-mitchell jessica-mitchell marked this pull request as draft August 22, 2022 09:04
@jessica-mitchell jessica-mitchell marked this pull request as ready for review August 24, 2022 19:31
@jessica-mitchell
Copy link
Contributor Author

@heplesser I looked into this more and discovered that basically the broken equations were missing $$. This seems to resolve the HTML rendering without affecting the output in a notebook running in Jupyter lab. I also updated the mathjax js in the conf, which I dont think did anything but figured we should avoid using outdated versions.

@jessica-mitchell
Copy link
Contributor Author

@heplesser I'm afraid there are some further issues after checking Read the docs rendering which do not seem straightforward

@jessica-mitchell jessica-mitchell marked this pull request as ready for review September 2, 2022 07:12
@med-ayssar
Copy link
Contributor

In The neuron emits a single spike if we should a line break before the unordered list, otherwise; it won't be rendered as a list.
Same for Upon spike emission

@med-ayssar
Copy link
Contributor

Side note, the space and position of the references at the bottom do not seem right.

Space problem in aeif_psc_delta_clopath.html

Position Problem in siegert_neuron.html

Why not simply use the horizontal representation for the references.

@jessica-mitchell
Copy link
Contributor Author

Space problem in aeif_psc_delta_clopath.html

Can you be more specific? I see a broken link in the third reference ...

Position Problem in siegert_neuron.html

Again, I dont see a problem, can you provide more details or screen shot?

Why not simply use the horizontal representation for the references.

What do you mean by horizontal representation?

@med-ayssar
Copy link
Contributor

I do not know if the problem of some settings I have in my local machine, but when building the docs, the references look like that:
Screenshot from 2022-11-15 10-37-30

They are vertical (going from top to bottom), and there is not enough space between columns. I tried building the docs with the current master and also this branch, and I have the same output.

@med-ayssar
Copy link
Contributor

med-ayssar commented Nov 15, 2022

I just found another list that is not rendered correctly.

Missing new line before start of list at Line 250 and at Line 1233

$I_h$ is not always written in latex mode. (see Line 365, Line 911 and Line 1058)

$h_{\infty}$ and $m_{\infty}$ also are not in latex mode: Line 1061

Copy link
Contributor

@med-ayssar med-ayssar left a comment

Choose a reason for hiding this comment

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

Otherwise, the rest looks good to me.

@jessica-mitchell
Copy link
Contributor Author

They are vertical (going from top to bottom), and there is not enough space between columns. I tried building the docs with the current master and also this branch, and I have the same output.

Ah this problem, yes this is rendered properly on Read the docs so I didnt notice it, but you're right, in the local build, something strange happens. I have it on my to do list to look into.

@jessica-mitchell
Copy link
Contributor Author

Fixes #2404

Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

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

@jessica-mitchell Looks good overall, just a few smaller comments.

doc/htmldoc/model_details/noise_generator.ipynb Outdated Show resolved Hide resolved
doc/htmldoc/model_details/noise_generator.ipynb Outdated Show resolved Hide resolved
doc/htmldoc/model_details/noise_generator.ipynb Outdated Show resolved Hide resolved
doc/htmldoc/model_details/noise_generator.ipynb Outdated Show resolved Hide resolved
doc/requirements.txt Outdated Show resolved Hide resolved
Documentation automation moved this from In progress to Review Nov 30, 2022
Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

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

Looks good now except for formatting error and merge conflict.

@jessica-mitchell
Copy link
Contributor Author

@med-ayssar Can you do a final review or approve if looks good? Thanks!

@med-ayssar
Copy link
Contributor

Looks good!

@heplesser heplesser merged commit 1966368 into nest:master Dec 6, 2022
Documentation automation moved this from Review to Done Dec 6, 2022
@jessica-mitchell jessica-mitchell deleted the fix-notebooks-docs branch April 17, 2024 18:11
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: Maintenance Work to keep up the quality of the code and documentation.
Projects
Documentation
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants