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

Documentation sidebar restructuring #1801

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

dana-gill
Copy link

@dana-gill dana-gill commented Jan 29, 2024

Reference Issue

Fixes #1478

What does this implement/fix? Explain your changes.

This restructures the Core IO section of documentation sidebar. Core IO no longer exists and is replaced by the following:

  • Signals (includes load, stream, resample, and any signal generators and helper utilities)
  • Spectral representations
  • Magnitude scaling
  • Unit conversion (time, frequency)
  • Music notation
  • Pitch and tuning
  • Harmonics
  • Phase recovery
  • Miscellaneous (collects current misc + frequency range generators)

Any other comments?

None

Also removes Phase Recovery from the Core IO and DSP section.
Also removes Magnitude Scaling from the Core IO and DSP section.
Copy link

codecov bot commented Jan 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.75%. Comparing base (8dae20c) to head (0120fcd).
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1801      +/-   ##
==========================================
- Coverage   98.77%   98.75%   -0.03%     
==========================================
  Files          34       35       +1     
  Lines        4666     4641      -25     
==========================================
- Hits         4609     4583      -26     
- Misses         57       58       +1     
Flag Coverage Δ
unittests 98.75% <ø> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Also removes Spectral Representation from the Core IO and DSP section.
Also removes Unit Conversion from the Core IO and DSP section.
Also removes Music Notation from the Core IO and DSP section.
Also removes Pitch and Tuning from the Core IO and DSP section.
Also removes Harmonics from the Core IO and DSP section.
@bmcfee
Copy link
Member

bmcfee commented Feb 21, 2024

Rebasing this from main should fix the build issues.

@bmcfee bmcfee added the documentation Issues relating to docstrings, examples, and documentation build label Feb 21, 2024
@dana-gill dana-gill force-pushed the documentation_sidebar_restructuring branch 2 times, most recently from 7b972dc to d075786 Compare March 27, 2024 16:06
Also removes Signals from the Core IO and DSP section. Audio Loading is
also part of Signals.
Also removes Miscellaneous from the Core IO and DSP section. Frequency
range generation is also included in Miscellaneous.
@dana-gill dana-gill force-pushed the documentation_sidebar_restructuring branch from d075786 to 0120fcd Compare March 27, 2024 17:09
@dana-gill dana-gill marked this pull request as ready for review March 27, 2024 17:10
@dana-gill
Copy link
Author

Alright @bmcfee, apologies for the delay, here is the PR :)

@bmcfee
Copy link
Member

bmcfee commented Mar 27, 2024

Thanks @dana-gill !

A couple of broad comments:

  • Can you rebase this on the current main? I made a couple of changes to the environment and sphinx config lately that this is presently reverting.
  • The new structure has lost the autosummary / menu list of subsection contents, eg https://librosa.org/doc/latest/core.html . Can we recover that?
  • We've also lost the module-level docstring, and the linter is now complaining about it.
  • I don't think it's correct to directly include generated contents (eg in harmonics.rst). Is there a way to keep this as using autodoc/ autosummary, but just break out the sections differently from what we had before? I think this should be as simple as keeping the module-level docstring in __init__.py, but just reorganizing the section information in the autosummary. Maybe I'm missing something?

@dana-gill
Copy link
Author

Can you rebase this on the current main? I made a couple of changes to the environment and sphinx config lately that this is presently reverting.

Huh, I thought I rebased it already. These are the commands I ran:

$ git fetch --all
$ git rebase origin/main

And it says Current branch documentation_sidebar_restructuring is up to date.. Are there any specific commands I should run for the repo?

The new structure has lost the autosummary / menu list of subsection contents, eg https://librosa.org/doc/latest/core.html . Can we recover that?

Yes we can 🥳 Just to make sure I understand you: You would like to the sidebar to have both the content of Core IO and DSP and the following (ie: How the branch looks now):

Signals (includes load, stream, resample, and any signal generators and helper utilities)
Spectral representations
Magnitude scaling
Unit conversion (time, frequency)
Music notation
Pitch and tuning
Harmonics
Phase recovery
Miscellaneous (collects current misc + frequency range generators)

Let me know if I understood that properly.

We've also lost the module-level docstring, and the linter is now complaining about it.

To make sure I understand: The linter would like librosa/__init__.py to stay the same and all of this should remain the same? https://github.com/librosa/librosa/blob/main/librosa/__init__.py#L3-L210

I don't think it's correct to directly include generated contents (eg in harmonics.rst). Is there a way to keep this as using autodoc/ autosummary, but just break out the sections differently from what we had before? I think this should be as simple as keeping the module-level docstring in init.py, but just reorganizing the section information in the autosummary. Maybe I'm missing something?

I need to do a bit more digging for this 😅 I think there was a reason why I couldn't just keep using autosummary, but I would have do some experimentations and get back to you.

@bmcfee
Copy link
Member

bmcfee commented Apr 4, 2024

You would like to the sidebar to have both the content of Core IO and DSP and the following (ie: How the branch looks now):

Signals (includes load, stream, resample, and any signal generators and helper utilities) Spectral representations Magnitude scaling Unit conversion (time, frequency) Music notation Pitch and tuning Harmonics Phase recovery Miscellaneous (collects current misc + frequency range generators)

Let me know if I understood that properly.

Currently all of those things are buried under the "Core IO and DSP" heading, which is not too accurate of a descriptor at this point. I think the idea here was to lift up the sub-headings of core (currently "audio loading", "time-domain processing", etc) to the top-level and eliminate the "core IO and dsp" section, along with some restructuring of the topics to make it a bit more navigable (as you describe).

To make sure I understand: The linter would like librosa/__init__.py to stay the same and all of this should remain the same? https://github.com/librosa/librosa/blob/main/librosa/__init__.py#L3-L210

Well the linter wants some docstring in the top-level module, but it doesn't necessarily care about the contents. In light of the above though, I think what we can do is just drop the "Core IO and DSP" heading, then replace the --- (second-level subheadings) by === (first-level headings) along with some reorganization as noted above. We shouldn't really need to change anything outside of __init__.py to get the new sidebar structure.

All that said, we should also while we're here consider #1823 - this AFAIK would just entail fleshing out the docstring for the core module with some descriptive text beyond just the menu headings. Also happy to take that up in a separate issue to keep things simple.

@dana-gill
Copy link
Author

You would like to the sidebar to have both the content of Core IO and DSP and the following (ie: How the branch looks now):
Signals (includes load, stream, resample, and any signal generators and helper utilities) Spectral representations Magnitude scaling Unit conversion (time, frequency) Music notation Pitch and tuning Harmonics Phase recovery Miscellaneous (collects current misc + frequency range generators)
Let me know if I understood that properly.

Currently all of those things are buried under the "Core IO and DSP" heading, which is not too accurate of a descriptor at this point. I think the idea here was to lift up the sub-headings of core (currently "audio loading", "time-domain processing", etc) to the top-level and eliminate the "core IO and dsp" section, along with some restructuring of the topics to make it a bit more navigable (as you describe).

To make sure I understand: The linter would like librosa/__init__.py to stay the same and all of this should remain the same? https://github.com/librosa/librosa/blob/main/librosa/__init__.py#L3-L210

Well the linter wants some docstring in the top-level module, but it doesn't necessarily care about the contents. In light of the above though, I think what we can do is just drop the "Core IO and DSP" heading, then replace the --- (second-level subheadings) by === (first-level headings) along with some reorganization as noted above. We shouldn't really need to change anything outside of __init__.py to get the new sidebar structure.

All that said, we should also while we're here consider #1823 - this AFAIK would just entail fleshing out the docstring for the core module with some descriptive text beyond just the menu headings. Also happy to take that up in a separate issue to keep things simple.

Ahh. So you would like the sidebar to look how it looks like in this PR and also have core.html at the same time?

I also did some investigation to see if it was possible to edit only the __init__.py and not add anrst files. It is, but you will have the plus icons on the left side. If that works with you, then I can make the changes :)

Screenshot 2024-04-05 at 12 55 57 PM

All that said, we should also while we're here consider #1823 - this AFAIK would just entail fleshing out the docstring for the core module with some descriptive text beyond just the menu headings. Also happy to take that up in a separate issue to keep things simple.

Sure! I can make that part of the PR.

@bmcfee
Copy link
Member

bmcfee commented Apr 5, 2024

Ahh. So you would like the sidebar to look how it looks like in this PR and also have core.html at the same time?

Yeah I suppose so. core.html is autogenerated from the top-level package import, ie the docstring in librosa/__init__.py, so all we're really talking about here is promoting the sub-headings within core up a level by changing the formatting.

I also did some investigation to see if it was possible to edit only the __init__.py and not add anrst files. It is, but you will have the plus icons on the left side. If that works with you, then I can make the changes :)

Yeah, that's totally fine! Thanks.

@bmcfee
Copy link
Member

bmcfee commented May 2, 2024

👋 @dana-gill just checking in to see how you're doing on this? It's the last PR we need to put in before pushing out the 0.10.2 release, so if there's anything I can do to help get it moving, please let me know.

@dana-gill
Copy link
Author

👋 @dana-gill just checking in to see how you're doing on this? It's the last PR we need to put in before pushing out the 0.10.2 release, so if there's anything I can do to help get it moving, please let me know.

Hey Brian! Thanks for checking in. I've been incredibly swamped with work so I don't think this will make it to the 0.10.2 release. Is that okay?

@bmcfee
Copy link
Member

bmcfee commented May 2, 2024

Ok, no worries! I'll make some today to sort out what actually needs to be there from what's a nice-to-have, and take this one out of the critical path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Issues relating to docstrings, examples, and documentation build
Development

Successfully merging this pull request may close these issues.

Documentation restructuring (core submodule)
2 participants