Skip to content

Restructure API Reference content to improve left sidebar #1598

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

Merged
merged 7 commits into from
Dec 20, 2022

Conversation

kandersolar
Copy link
Member

@kandersolar kandersolar commented Nov 22, 2022

  • Closes #xxxx
  • I am familiar with the contributing guidelines
  • Tests added
  • Updates entries in docs/sphinx/source/reference for API changes.
  • Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for all changes. Includes link to the GitHub Issue with :issue:`num` or this Pull Request with :pull:`num`. Includes contributor name and/or GitHub username (link with :ghuser:`user`).
  • New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

The sidebar in the pydata-sphinx-theme reflects the document hierarchy; it shows the tree relationship between parent pages and children pages. Unfortunately, unlike the RTD theme we used to use, it doesn't seem to have any ability to group siblings by what section of the page their toctree entry appears in. That results in unwieldy layers of the tree with 50+ elements. I think this is what @wholmgren was referring to in #329 (comment).

I have looked into this at some length and I don't think there's any way to somehow include the in-page header/layout information in the left sidebar. It is possible to show captions for the top-level tree nodes as I did for bifacial_radiance, but I don't think that can be used anywhere but the top level, plus they can't expand/collapse their lists. The only way I have found to more or less replicate our sidebar from the RTD theme is to rearrange the content such that the document (folder) hierarchy reflects the desired structure. The first commit on this PR branch tries it out for the PV Modeling subsection of our API Reference:

main (link) this PR (link)
image image

Unfortunately this also means that things that used to be all on one page (e.g. the IAM and Temperature toctrees) are now separate pages. I don't think there's any way around this with the pydata theme; if we want a tree-like sidebar then we have to have a tree-like page hierarchy.

Switching to another theme is another possibility, but I'm not enthusiastic about that idea.

For now I've only changed the "PV Modeling" section in order to get feedback on this approach. If it is accepted then I will proceed with restructuring the other sections as well.

The left sidebar in pydata-sphinx-theme reflects document hierarchy, without considering in-page headers.  To get those headers into the sidebar, we have to rework the document hierarchy such that the former in-page headers are now reflected in the document structure.  Not ideal, but it's the only way I've found so far.
@kandersolar kandersolar added this to the 0.9.4 milestone Nov 22, 2022
@adriesse
Copy link
Member

I think it is an improvement. Independent of this, there are some awkward categories and this may or may not be the right time or place to discuss them. Some things are categorized by what they model; others by how they model something. I'm not sure, for example, where empirical models of PV module behavior would fit.

@AdamRJensen
Copy link
Member

After having looked into this a bit, there doesn't seem to be a better solution than what @kanderso-nrel proposes. Thus, I am in favor of changing the document/file structure

@mikofski
Copy link
Member

mikofski commented Dec 7, 2022

@kanderso-nrel I like the new layout too.

@kandersolar kandersolar mentioned this pull request Dec 9, 2022
9 tasks
@mikofski
Copy link
Member

I like it a lot!

@kandersolar kandersolar marked this pull request as ready for review December 13, 2022 16:31
@kandersolar
Copy link
Member Author

Thanks everyone for taking a look. I broke up the pv_modeling, effects_on_pv_system_output, and irradiance pages since they had in-page headers that could be recast as directory structure. modelchain is another candidate but I thought it would be better to keep all that in one page since those class methods are more related to each other than the functions on the other pages are. The other pages didn't have in-page headers so I left them alone.

As always there's more that could be done to improve the Reference section, but I'm inclined to call this an improvement as-is and leave other changes to the future. Test failure is unrelated.

@cwhanse
Copy link
Member

cwhanse commented Dec 15, 2022

@kanderso-nrel this is a significant improvement to the API reference. I have more hope that I can locate the page for a specific function.

@kandersolar
Copy link
Member Author

Ok, glad to hear it. I'm waiting to finalize this PR until #1602 is merged; better to resolve these pesky conflicts here in this PR. After that I'll submit a PR for the release itself.

@adriesse
Copy link
Member

One thing that is not clear is where various existing and future fitting functions should go. In general I think the best place for them would be as close as possible to the models they serve, both in the code (except if they are really long) and in the API reference. There could be a section listing just fitting functions as there is now, but I would see that as secondary.

@kandersolar
Copy link
Member Author

I'm pretty sure I've correctly fixed up all the merge conflicts here, but of course we should expect additional conflicts in any open PRs that touch this part of the docs. I won't bother folks for a green checkmark before merging since several people have already expressed support.

Let's take up @adriesse's point about where functions should live and be listed in a separate issue (Anton, can you open one?).

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

Successfully merging this pull request may close these issues.

5 participants