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

update ophys tutorial #1375

Merged
merged 11 commits into from
Aug 10, 2021
Merged

update ophys tutorial #1375

merged 11 commits into from
Aug 10, 2021

Conversation

bendichter
Copy link
Contributor

@bendichter bendichter commented Jun 23, 2021

Motivation

The pynwb ophys tutorial is outdated. We need to update with content and figures from nwb_tutorial ophys tutorial.

To render changes, run make htmldoc in terminal.

You may need to remove the allensdk requirement first in order to get it to run, but don't commit that change.

@codecov
Copy link

codecov bot commented Jun 23, 2021

Codecov Report

Merging #1375 (0fbdd41) into dev (c0ad3e7) will decrease coverage by 3.25%.
The diff coverage is n/a.

❗ Current head 0fbdd41 differs from pull request most recent head a8b1435. Consider uploading reports for the commit a8b1435 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #1375      +/-   ##
==========================================
- Coverage   76.78%   73.53%   -3.26%     
==========================================
  Files          37       37              
  Lines        2731     2331     -400     
  Branches      455      357      -98     
==========================================
- Hits         2097     1714     -383     
- Misses        552      556       +4     
+ Partials       82       61      -21     
Impacted Files Coverage Δ
src/pynwb/io/icephys.py 66.66% <0.00%> (-33.34%) ⬇️
src/pynwb/io/base.py 30.50% <0.00%> (-25.31%) ⬇️
src/pynwb/file.py 82.75% <0.00%> (-3.61%) ⬇️
src/pynwb/io/file.py 80.39% <0.00%> (-1.89%) ⬇️
src/pynwb/base.py 92.85% <0.00%> (-1.83%) ⬇️
src/pynwb/image.py 94.44% <0.00%> (-0.62%) ⬇️
src/pynwb/misc.py 87.31% <0.00%> (-0.10%) ⬇️
src/pynwb/behavior.py 100.00% <0.00%> (ø)
src/pynwb/io/image.py 66.66% <0.00%> (ø)
src/pynwb/validate.py 0.00% <0.00%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c0ad3e7...a8b1435. Read the comment docs.

@rly
Copy link
Contributor

rly commented Jun 24, 2021

I fixed it. Not sure if there is a better way than putting the image in _static, but here is one example of sphinx gallery doing it that way:
https://sphinx-gallery.github.io/stable/configuration.html#adding-images-to-notebooks

I have not tested whether the image appears in the generated notebooks.

Also see warning C:\Users\Ryan\Documents\NWB\pynwb\docs\source\tutorials\domain\ophys.rst:359: WARNING: Too many autonumbered footnote references: only 3 corresponding footnotes available.

@oruebel
Copy link
Contributor

oruebel commented Jun 24, 2021

@rly @bendichter Note, the SVG figure will most likely not work right now with non-HTML formats (e.g., PDF etc.) that are being generated on readthedocs. For this to work we should either add a PNG version of the figure as well so that we can use the SVG in HTML and otherwise use the PNG version or add the "sphinx.ext.imgconverter" if that that extension is working on readthedocs. See sphinx-doc/sphinx#1907

@bendichter
Copy link
Contributor Author

@oruebel I understand that outputting the docs as pdfs is a feature sphinx supports, but does anyone use it? I think it makes sense to optimize for website content, since that's what our users have access to

@oruebel
Copy link
Contributor

oruebel commented Jun 24, 2021

I understand that outputting the docs as pdfs is a feature sphinx supports, but does anyone use it

ReadTheDocs automatically generates the PDFs and they are available for download. I don't have a measure for how many folks are downloading the PDF or are generating it themselves. I think including the image as both SVG and PNG in the docs should be simple enough and then we can simply do the following to keep the docs working in all format.

.. only:: html
    show svg figure

.. only:: not html
   show png figure

@bendichter bendichter changed the title failing attempt to add image to ophys tutorial add image to ophys tutorial Jun 24, 2021
@bendichter bendichter changed the title add image to ophys tutorial update ophys tutorial Jul 23, 2021
@weiglszonja
Copy link
Collaborator

I'm working on updating the content and figures; however I have an issue when trying to view the changes with make htmldoc. I commented out allensdk from the docs requirements just as @bendichter suggested, however there is a configuration error:

Configuration error:
There is a programmable error in your configuration file:
  File "/Users/weian/venv/pynwb/lib/python3.7/site-packages/hdmf/spec/namespace.py", line 316, in get_spec
    raise KeyError("'%s' not a namespace" % namespace)
KeyError: "'core' not a namespace"

I installed pynwb after cloning the repository with pip install -e . Installing it from pip results in the same error.
@rly @oruebel Do you have any suggestions what causes this error?

I copied the full traceback here:

Traceback (most recent call last):
  File "/Users/weian/venv/pynwb/lib/python3.7/site-packages/sphinx/config.py", line 327, in eval_config_file
    exec(code, namespace)
  File "/Users/weian/catalystneuro/pynwb/docs/source/conf.py", line 35, in <module>
    from pynwb._version import get_versions
  File "/Users/weian/catalystneuro/pynwb/src/pynwb/__init__.py", line 260, in <module>
    from . import io as __io  # noqa: F401,E402
  File "/Users/weian/catalystneuro/pynwb/src/pynwb/io/__init__.py", line 1, in <module>
    from . import base as __base
  File "/Users/weian/catalystneuro/pynwb/src/pynwb/io/base.py", line 1, in <module>
    from .core import NWBContainerMapper
  File "/Users/weian/catalystneuro/pynwb/src/pynwb/io/core.py", line 9, in <module>
    from pynwb.file import NWBFile
  File "/Users/weian/catalystneuro/pynwb/src/pynwb/file.py", line 13, in <module>
    from .base import TimeSeries, ProcessingModule
  File "/Users/weian/catalystneuro/pynwb/src/pynwb/base.py", line 9, in <module>
    from .core import NWBDataInterface, MultiContainerInterface, NWBData
  File "/Users/weian/catalystneuro/pynwb/src/pynwb/core.py", line 36, in <module>
    class NWBContainer(NWBMixin, Container):
  File "/Users/weian/catalystneuro/pynwb/src/pynwb/__init__.py", line 130, in _dec
    __TYPE_MAP.register_container_type(namespace, neurodata_type, cls)
  File "/Users/weian/venv/pynwb/lib/python3.7/site-packages/hdmf/utils.py", line 580, in func_call
    return func(args[0], **pargs)
  File "/Users/weian/venv/pynwb/lib/python3.7/site-packages/hdmf/build/manager.py", line 725, in register_container_type
    spec = self.__ns_catalog.get_spec(namespace, data_type)  # make sure the spec exists
  File "/Users/weian/venv/pynwb/lib/python3.7/site-packages/hdmf/utils.py", line 580, in func_call
    return func(args[0], **pargs)
  File "/Users/weian/venv/pynwb/lib/python3.7/site-packages/hdmf/spec/namespace.py", line 316, in get_spec
    raise KeyError("'%s' not a namespace" % namespace)
KeyError: "'core' not a namespace"

Thank you for the help!

@oruebel
Copy link
Contributor

oruebel commented Aug 2, 2021

@weiglszonja I think you may be missing the git submodules for the nwb-schema in your instillation. Did you include the --recurse-submodules option when you cloned PyNWB? I.e., did you clone the repo as follows?

git clone --recurse-submodules [email protected]:NeurodataWithoutBorders/pynwb.git

The instructions for installing from Git are here https://pynwb.readthedocs.io/en/stable/getting_started.html#install-from-git-repository

@weiglszonja
Copy link
Collaborator

@oruebel Thanks for the quick reply! you're right, I forgot to add the submodules option. It works now, thank you!

* update ophys tutorial from nwb_tutorial notebook

* add ophys tutorial SVG files
@bendichter
Copy link
Contributor Author

bendichter commented Aug 2, 2021

@weiglszonja this looks great!

  • Can you solve the flake8 error?
  • Can you center the images?
  • In the first paragraph, can you make it a link to the NWB Basics tutorial?

Does anything else need to be done for this tutorial?

bendichter and others added 2 commits August 2, 2021 16:56
* update ophys tutorial from nwb_tutorial notebook

* add ophys tutorial SVG files

* fix flake8 over-indent error

* align images to center

* fix NWBFile class reference

* cross-reference NWB Basics tutorial

* fix compatibility with generating pdfs from docs

* fix too short lines under title

* add PNG files
@bendichter bendichter marked this pull request as ready for review August 5, 2021 15:58
@bendichter bendichter requested a review from rly August 5, 2021 22:05
@oruebel
Copy link
Contributor

oruebel commented Aug 10, 2021

BTW, I noticed you are using matplotlib in the tutorial. If you want the plots to show up in the HTML docs (and on ReadTheDocs) then simply renaming the file to add the prefix plot_ should do the trick (e.g., you could do git mv docs/gallery/domain/ophys.py docs/gallery/domain/plot_ophys.py). SphinxGallery uses the prefix as an indicator to decide whether to capture the outputs of the gallery file (similar to a Jupyter Notebook) or to just run and render the file without showing the outputs of the different code blocks.

@rly
Copy link
Contributor

rly commented Aug 10, 2021

@bendichter this is good to go!

@bendichter bendichter merged commit 3760038 into dev Aug 10, 2021
@rly rly deleted the merge_ophys_tutorials branch August 10, 2021 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants