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

Reorganising documentation into Getting Started, Tutorial and How To #2778

Merged
merged 18 commits into from
May 9, 2024

Conversation

chrishalcrow
Copy link
Collaborator

@chrishalcrow chrishalcrow commented Apr 29, 2024

New documentation structure, based on discussions in #2763 and #2327.

The reorganised docs:
https://spikeinterface--2778.org.readthedocs.build/en/2778/

Idea is to make the documentation simpler to navigate for new (to si and to coding) users. Main toctree is

  • Overview
  • Getting Started
  • Tutorials
  • How to...
  • In-depth
  • API
  • as before...

I deliberately just changed the structure and moved files around, and didn't change the files themselves (except some titles, and descriptions on index pages). Once this overall structure is fixed, we can have other debates about what belongs where, and what is missing.

The tutorials are generated by sphinx-gallery, but contain some "plain" rst files. This works, but the two types are linked from index in two different ways. I think this is fine.

The build looks fine locally, but I'm sure there will be some broken things.

Not sure about the name "In-depth". I quite like "How to...". Opinions welcome on everything!
@h-mayorquin @zm711 @JoeZiminski @alejoe91 @samuelgarcia

@zm711
Copy link
Collaborator

zm711 commented Apr 29, 2024

image

image

Just going to post the pic so we can see what it looks like along the side in the actual build.

@chrishalcrow
Copy link
Collaborator Author

@JoeZiminski suggested putting the Viewers/Visualising_data section in the Tutorials. Thoughts?

@zm711
Copy link
Collaborator

zm711 commented Apr 29, 2024

They aren't super informative (I mean depth-wise), so I would probably think of them more like "how to". Ie how to visualize your data rather than an in-depth visualization tutorial.

As an aside: For me In depth is hard to parse. I have no clue what that means. I think Modules Documentation would be clearer unless someone has a better name.

@alejoe91 alejoe91 added the documentation Improvements or additions to documentation label Apr 30, 2024
@JoeZiminski
Copy link
Collaborator

Agree @zm711 'Visualising Data' makes sense in 'How To...'. (maybe renamed to 'Visualise Data').

@h-mayorquin
Copy link
Collaborator

OK, I gave it some thought to this today.
Now that we have green light by both Sam and Alessio we should try to get a version of this working quickly and then improve after. The lowest hanging fruit is to not have both the "modules documentation" and the "modules examples gallery at the same level. This PR does this and therefore it is better than what we have already.

I would even vote to merge as it is and then we can discuss after as the rest. Another option is to make a PR where that is the only change. What do you think?

That said, my opinion as I have read the documentation here and checked how some standard packages (numpy, scipy, scikit-learn, etc) handle their docs:

  • Change "how to get started" section that is nessted in "Getting Started" to "quick start". This follow numpy convention https://numpy.org/doc/stable/user/quickstart.html and also avoids this akward (imo) redundancy.
  • Visualizaing data is too short right now to be its own section. I would put it a section of "getting started" as a pointer and hopefully later we can expand on it and see where it fits better.
  • Again, following Numpy my suggestion for "In-depth" would be "Spikeinterface Fundamentals"
  • I don't think that the section of "Tutorials without a notebook" is a good section. The neuroxpiel too is a how-to (it solves a specific problem) and I think the "From WaveformExtractor to SortingAnalyzer" can be a section on its own called "updating from legacy" or something like that.
  • Lastly, I would really like to not use mearec on the "get started", reading the "get started" I think we can use the generator for that but this can be another PR.

I think that big limitation that we have is the organization the documentation that reflects the package organization (the modules). This is easy for us programmers and a good start but overall I don't think it is a good organization principle. Users care about objects and we should center around those objects and what they make affordable. There are modules that maybe we don't want to document, and we don't need to explain everything about a module.

@JoeZiminski
Copy link
Collaborator

JoeZiminski commented May 2, 2024

EDIT: This post was a (longer than anticipated) post on 'Modules Documentation' which is now moved to #2800.

Copy link
Collaborator

@JoeZiminski JoeZiminski left a comment

Choose a reason for hiding this comment

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

Hey @chrishalcrow this looks great, just a few small suggestions. I agree with @h-mayorquin this could be merged pretty much as-is.


installation
import
get_started
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make sense to change the order to:

  • installation
  • get_started
  • import
  • install_sorters

(just because new user is probably going to want to get an overview on getting started rather than dive into details on the imports).

Could 'How to "get started"' be renamed, maybe to 'Getting Started Tutorial'?

Copy link
Collaborator

Choose a reason for hiding this comment

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

actually @h-mayorquin 'quickstart' is good and avoids the redundancy between section name and tutorial name. Maybe 'Quickstart Guide' or 'Quickside Tutorial' just because 'quickstart' on it's own might be slightly amiguous?

@@ -0,0 +1,338 @@
.. _installsorters:
Copy link
Collaborator

@JoeZiminski JoeZiminski May 2, 2024

Choose a reason for hiding this comment

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

I wonder if this page can also be moved, maybe to what is currently 'In-Depth'? As the use of docker images is recommended in the article this is less general-use than originally anticipated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

On further consideration I don't think now is the time to do it, and it wouldn't really go with the rest of 'In Depth', so makes sense to ignore this for now.

@@ -0,0 +1,81 @@
Importing SpikeInterface
Copy link
Collaborator

Choose a reason for hiding this comment

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

As a general point I think the 'Importing SpikeInterface' page is confusing for new users and should probably go under developer docs. Maybe it is a good time to finalise the discussion from #2046. It is useful to have a place to discuss the different ways of importing but I don't think new users should be concerned with it. They should be recommended a way to a) reduce mental load b) ensure a consistency in how the package is used across the community.

For example SciPy have a discussion on importing in their API docs but in the User Guide they just introduce a consistent way from the start and don't discuss.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That discussion will never be solved haha so this doc was the compromise :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Haha yes I had forgotton the extent of the discussion 😄 Okay makes sense

doc/how_to/index.rst Outdated Show resolved Hide resolved
@zm711
Copy link
Collaborator

zm711 commented May 2, 2024

My vote would be these 3 changes and then do a better cleanup either 1) after the conference or 2) hackathon

Change "how to get started" section that is nessted in "Getting Started" to "quick start". This follow numpy convention https://numpy.org/doc/stable/user/quickstart.html and also avoids this akward (imo) redundancy.

Visualizaing data is too short right now to be its own section. I would put it a section of "getting started" as a pointer and hopefully later we can expand on it and see where it fits better.

Again, following Numpy my suggestion for "In-depth" would be "Spikeinterface Fundamentals"

@JoeZiminski
Copy link
Collaborator

Agree with @h-mayorquin and @zm711 my only thought is 'Visualising Data' could go in 'How To' rather than 'Getting Started' as I think unless you are familiar with SI the article will not make much sense.

@zm711
Copy link
Collaborator

zm711 commented May 2, 2024

@JoeZiminski you want to post your big comment somewhere else? Maybe one of the document issues. I think it has a lot of wonderful food for thought for the next phase of doc edits and I don't want to lose it when this merged.

@JoeZiminski
Copy link
Collaborator

Sure thanks @zm711, do you think a new issue would make sense?

@zm711
Copy link
Collaborator

zm711 commented May 2, 2024

I think a new issue would be fine. We can have a fresh start!

@h-mayorquin
Copy link
Collaborator

Thinking twice I don't think we should merge this yet. I think we should remove the generated files out of version control.

@chrishalcrow
Copy link
Collaborator Author

Lots of discussion, great!

I'll implement the generally agreed about stuff later, then need to check the version control stuff (good catch @h-mayorquin), and try to clean up references to the old Section names throughout the docs. And we'll talk about what goes where and what can be split-up etc in person in Edinburgh?

Talking of names, seems like "Module documentation" still wins. Let's keep it for now? @alejoe91 suggested "Module by Module" as a name for "Module documentation". I thought it sounded terrible at first, but woke up thinking it was a nice name.

I don't think that the section of "Tutorials without a notebook" is a good section. The neuroxpiel too is a how-to (it solves a specific problem) and I think the "From WaveformExtractor to SortingAnalyzer" can be a section on its own called "updating from legacy" or something like that.

Agreed. I wanted to include this as it shows the stylistic constraints if we use sphinx-gallery: we can include basic rst files, but the way they're included means it's not easy to bundle them with the gallery tutorials. So their either in their own section (here in the "Tutorials without a notebook" section) or could be in a subsection, but would appear as a list of links instead of a gallery card. But you're right, for these examples are there are easy fixes: have neuropxiles as a how-to and put "WE to SA" in its own section.

@JoeZiminski
Copy link
Collaborator

JoeZiminski commented May 3, 2024

Sounds good @chrishalcrow!

Happy to keep 'Module Documentation' for now. In general my thoughts are to keep the documentation as accessible as possible, as many users may not be very experienced with python. Imagining yourself as a new user with limited python experience navigating the website for the first time, anything with 'Module' in the name is not informative. Obviously the docs will need to get technical in places but not on the front page. So I would advocate for changing it later but happy to keep as is as there are many changes and discussions to come.

In terms of the timeline, for me personally I am away next week but then back on ephys stuff now hopefully for a long while and was planning to help with the documentation as I realised I've never read it all the way through. So for me it would work to continue discussion and refactorings up until the conference, and this may leave only the bigger things to tackle in person. But happy to proceed in whatever way works best for everyone!

zm711
zm711 previously requested changes May 3, 2024
Copy link
Collaborator

@zm711 zm711 left a comment

Choose a reason for hiding this comment

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

Just putting a request changes because I noticed we are clobbering a recent doc PR about one of the quality metrics. See comment. We definitely can't merge until we fix that!

@chrishalcrow chrishalcrow marked this pull request as ready for review May 3, 2024 13:00
@chrishalcrow
Copy link
Collaborator Author

Wait! I've done a stupid thing with the analyze neuropixles how to... DON'T MERGE

@JoeZiminski
Copy link
Collaborator

Thanks Chris! I kind of ignored .rst changes for some reason I thought they were autogenerated by sphinx-build but I see from @zm711 the order of merges could affect things. Just checking some changes seen are intended, I wasn't sure if they were intended rewrites or regression overwriting since-merged PRs:

  • core.rst rewrite
  • renaming of SortingAnalyzer to WaveformExtractor in exporters.rst
  • section removed from postprocessing.rst
  • qualitrymetrics/amplitude_cutoff.rst
  • rewrite, some small changes to plot_4_sorting_analyzer.py

@JoeZiminski
Copy link
Collaborator

BTW the title of this PR could be renamed before merge now that the structure is more decided. How about 'Reorganising documentation into Getting Started, Tutorial and How To.'?

@zm711
Copy link
Collaborator

zm711 commented May 3, 2024

Thanks Chris! I kind of ignored .rst changes for some reason I thought they were autogenerated by sphinx-build but I see from @zm711 the order of merges could affect things. Just checking some changes seen are intended, I wasn't sure if they were intended rewrites or regression overwriting since-merged PRs:

  • core.rst rewrite
  • renaming of SortingAnalyzer to WaveformExtractor in exporters.rst
  • section removed from postprocessing.rst
  • qualitrymetrics/amplitude_cutoff.rst
  • rewrite, some small changes to plot_4_sorting_analyzer.py

I think these could all be regressions! We will need to double check them carefully!

@chrishalcrow
Copy link
Collaborator Author

Hello. Yes, I must have messed something up early. It was hard to spot because I had also messed up the gitignore so it was hard to work through the diffs. I know that I didn't properly rerun the quickstart tutorial properly.

Now this PR is only 24 lines extra, which sounds very reasonable. Once it's finished running, could people have a final check? @h-mayorquin @zm711 @JoeZiminski

@chrishalcrow chrishalcrow changed the title Possible new documentation structure Reorganising documentation into Getting Started, Tutorial and How To May 3, 2024
Copy link
Collaborator

@h-mayorquin h-mayorquin May 3, 2024

Choose a reason for hiding this comment

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

This png are generated files. I think we should remove them from version control, right?

Copy link
Member

Choose a reason for hiding this comment

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

No they are used in the rst, so they need to be there

Copy link
Collaborator

Choose a reason for hiding this comment

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

But they are generated on the read the docs so they will be there for the rst to display. They just don't need to be in version control. Am I wrong?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are generated in a different way than the sphinx-gallery files. They're run locally so that we can do big computations and use larger data files. See here: https://github.com/SpikeInterface/spikeinterface/tree/main/examples/how_to
To be honest, they are a pain, but that's a discussion for another PR!

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, thanks for the pointer. I forgot that there is yet-another-system of generating files in the docs. Maybe the simplest if to just not change the images every time we run the code (they should be the same!). We should discuss this in another PR or issue though as this is creating the dreaded super large repo of @samuelgarcia .

@h-mayorquin
Copy link
Collaborator

h-mayorquin commented May 3, 2024

I checked generated docs and they look OK to me

Let's not forget to squash the commit history for this one!

I don't like "Modules Documentation" because I don't want to structure the documentation around modules. I want to add in depth sections about the sorting_vector and the parallel_processing for example and the current structure means that everything will be super full in core and kind of empty on extractors for example. But we can discuss this in another PR or issue once I get to formulate this better.

@zm711
Copy link
Collaborator

zm711 commented May 3, 2024

@h-mayorquin, Joe just opened a new issue to discuss a deeper rearrange once this toctree is done. You should read his takes and then add your own there! #2800.

@zm711 zm711 dismissed their stale review May 3, 2024 17:08

removing my request for new review.

examples/tutorials/README.rst Outdated Show resolved Hide resolved
doc/how_to/index.rst Outdated Show resolved Hide resolved
@zm711
Copy link
Collaborator

zm711 commented May 6, 2024

@chrishalcrow,

Just two typos and then this looks good to me (one comment that doesn't necessarily need to be addressed here).

@zm711
Copy link
Collaborator

zm711 commented May 7, 2024

This is good for me. Anything else you wanted @h-mayorquin ?

Copy link
Collaborator

@h-mayorquin h-mayorquin left a comment

Choose a reason for hiding this comment

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

No, this is fine for me.

@h-mayorquin h-mayorquin merged commit 7841341 into SpikeInterface:main May 9, 2024
11 checks passed
@h-mayorquin
Copy link
Collaborator

Thanks for the push for this @chrishalcrow

@chrishalcrow
Copy link
Collaborator Author

Thanks for getting this merged - I've been on holiday (I maybe should have mentioned that at some point!!)

Looking forward to much more documentation discussion in the next couple of months!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants