Skip to content
This repository has been archived by the owner on Mar 15, 2023. It is now read-only.

Conversion examples in the documentation and docstrings #495

Merged
merged 61 commits into from
May 11, 2022

Conversation

h-mayorquin
Copy link
Collaborator

@h-mayorquin h-mayorquin commented May 3, 2022

Motivation

@bendichter mentioned in of our team meetings that it would be an idea to have a gallery of examples that we could quicly point out to the users. Also, we have issue #442 to add examples in the docstrings of non-data. This PR is to test whether it is feasible to integrate both approaches and at the same subject the docstring and the example gallery to testing suite using the capabilities provided by doctest:

To-do:

  • Add an example to the gallery to showcase format and idea.
  • Test locally with test.
  • Add tests to CI.

After the gallery examples are done we can add links to each of the docstrings in their respective data-interfaces so the examples are accessible from there.

@h-mayorquin h-mayorquin added documentation Improvements or additions to documentation enhancement New feature or request high priority currently blocking an issue in another project. That issue should be referenced testing labels May 3, 2022
@h-mayorquin h-mayorquin self-assigned this May 3, 2022
@h-mayorquin h-mayorquin changed the title First draft of example gallery Conversion examples in the documentation and docstrings May 3, 2022
@bendichter
Copy link
Collaborator

consider using sphinx.ext.doctest

@h-mayorquin
Copy link
Collaborator Author

h-mayorquin commented May 4, 2022

consider using sphinx.ext.doctest

Yes, I am trying to find out if this can work as I expect that we can get prettier format that way. One problem with this sphinx.ext.doctest is that the tests are done at the time when the documentation is built. This is problematic for us because the gin_test files are not available in the read the docs environment and my impression is that we have way less control over that one than what we have for the CI environment here in github.

I am checking today if we can have sphinx.ext.doctest format but doing the testing with pytest in github CI.
Also, see #498.

@bendichter
Copy link
Collaborator

@h-mayorquin makes sense. I would avoid downloading data in the docs build because I think we'll run up against space and time contraints on readthedocs

@h-mayorquin h-mayorquin marked this pull request as ready for review May 5, 2022 13:01
@h-mayorquin
Copy link
Collaborator Author

h-mayorquin commented May 5, 2022

OK, so now we have a pipe-line going on here. I added testing in CI and now I added some utilities so the code can be display with and without the prompt (>>>), I also added copying functionality.

Right now, I have two format proposals in the neuroscope docs.

Could we chose the format between these two (or any other), accept this as a template to base future examples on and I will all the other interfaces based on this, @bendichter ?
I am taking a long flight tomorrow and I would be good to chose one now so I can add all the rest of them on the go : )

>>> # Creates a datetime object with date the first of Junuary of 2020 in the US/Pacific time-zone
>>> metadata = interface.get_metadata()
>>> session_start_time = datetime(2020, 1, 1, 12, 30, 0, tzinfo=tz.gettz("US/Pacific"))
>>> metadata["NWBFile"] = dict(session_start_time=session_start_time)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
>>> metadata["NWBFile"] = dict(session_start_time=session_start_time)
>>> metadata["NWBFile"].update(session_start_time=session_start_time)

I prefer this syntax. The way you have it here, any fields in NWBFile other than session_start_time would be removed. You could also do metadata["NWBFile"]["session_start_time"] = session_start_time, but I prefer doing the dictionary update because you can assign multiple fields at once if you want to, e.g.

metadata["NWBFile"].update(session_start_time=session_start_time, notes="hello")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, this makes more sense.
Done in the latest commit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bendichter
I just realized though, that this fails because the metadata does not necessarily have the key NWBFile at that point in time.

docs/conf.py Outdated Show resolved Hide resolved
True

The other type of display:
^^^^^^^^^^^^^^^^^^^^^^^^^^
Copy link
Collaborator

@bendichter bendichter May 5, 2022

Choose a reason for hiding this comment

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

I prefer this one. I expect most users to want to copy/paste the entire code block and this makes that a bit easier.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Ben, this is a short example, I don't think it's necessary to break it down into different code blocks.

I'd also format the comments so that the scrollbar disappears

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added the changes and now this is the only one that remains. Which scrollbar, @weiglszonja? (maybe it disappear now but I did not know what you were talking about).

@CodyCBakerPhD
Copy link
Member

Axona, blackrock, spikegadgets, spikeglx don't have spacing between comments consistent with the other interfaces (which looks slightly better)

OpenEphys doesn't have the same comment structure overall

Neuroscope and cellexplorer sorting examples?

@CodyCBakerPhD
Copy link
Member

(I think Axona also technically has a sorting interface, but it was more ad hoc as I recall so maybe skip for now?)

@@ -0,0 +1,32 @@
Conversion Gallery
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd put this file inside the conversion_examples_gallery directory. Then all of the reference can shorten, e.g. conversion_examples_gallery/recording/axona -> recording/axona

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking on having everything that is on index.rst (the top-level file) also in docs. The idea was the file structure would reflect the document structure.

@h-mayorquin
Copy link
Collaborator Author

Axona, blackrock, spikegadgets, spikeglx don't have spacing between comments consistent with the other interfaces (which looks slightly better)

OpenEphys doesn't have the same comment structure overall

Neuroscope and cellexplorer sorting examples?

Thanks, I align the comment structure for the former group.

OpenEphys produces output by default (click on the >>> to see what's need for testing).

Neuroscope and cellexplorer. Those ones are missing.

CodyCBakerPhD
CodyCBakerPhD previously approved these changes May 11, 2022
@CodyCBakerPhD CodyCBakerPhD merged commit 9a6da28 into main May 11, 2022
@CodyCBakerPhD CodyCBakerPhD deleted the add_conversion_in_the_docstring_of_data_interfaces branch May 11, 2022 21:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Improvements or additions to documentation enhancement New feature or request high priority currently blocking an issue in another project. That issue should be referenced testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants