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 dispersion docs #118

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Update dispersion docs #118

wants to merge 3 commits into from

Conversation

MarJMue
Copy link
Collaborator

@MarJMue MarJMue commented Feb 6, 2023

Followup to #113

I think we should make the distinction between index and dielectric dispersions clearer.

Two problems:

  • the sidebar does not show the deeper hierarchy
  • maybe we have to split the dispersions into more pages in future, to keep responsiveness

@domna
Copy link
Member

domna commented Feb 6, 2023

Yes, we also have to check the dispersion doc string and actual formula. There are definitely some errors in there (see also #55)

the sidebar does not show the deeper hierarchy

With deeper hierarchy you mean the hierarchy Dispersion -> IndexDispersion? I think it is very important to show the different behaviour and also explain why and that IndexDispersions cannot be added to other dispersions. For me I have the feeling that we should isolate the IndexDispersion as the special case and refer to the Dispersion as "normal" behaviour (which is also what we do in the code). Do you agree?

maybe we have to split the dispersions into more pages in future, to keep responsiveness

Just to understand clearly: You mean to splitting into group of dispersions, like empirical, physical etc. (of course we'd need to think what good categories would be)?

@MarJMue
Copy link
Collaborator Author

MarJMue commented Feb 8, 2023

With deeper hierarchy you mean the hierarchy Dispersion -> IndexDispersion?

If you include this distinction, which is as you say very important, you loose on a technical level the list of all dispersions in the sidebar. I liked the abilitiy to directly select the wanted dispersion. I just have not found the right setting inside sphinx to change that.

Just to understand clearly: You mean to splitting into group of dispersions, like empirical, physical etc. (of course we'd need to think what good categories would be)?

Maybe it is a problem on my side, but it takes a noticeable amount of time until all plotly graphics are loaded.
And as the number of dispersions will increase with time, we might find ways to split the list in chunks or use seperate pages per dispersion, akin to the pages for every numpy function.

@domna
Copy link
Member

domna commented Feb 8, 2023

If you include this distinction, which is as you say very important, you loose on a technical level the list of all dispersions in the sidebar. I liked the abilitiy to directly select the wanted dispersion. I just have not found the right setting inside sphinx to change that.

Yes I agree that it would be nice to keep this list on the side.

Maybe it is a problem on my side, but it takes a noticeable amount of time until all plotly graphics are loaded. And as the number of dispersions will increase with time, we might find ways to split the list in chunks or use seperate pages per dispersion, akin to the pages for every numpy function.

Yes, it takes a while for me as well. Additionally, some of the graphs are randomly not displayed for me (the grid is, however, but the line is missing). Typically, reloading resolves this issue. Maybe there is also the possibility of caching the graphs as I think rn they are re-rendered on every page load (but I'm not sure how this is resolved). I think there we may also find a technical optimisation, but I find splitting the dispersions into sub-pages also a good idea 👍️.

@domna
Copy link
Member

domna commented Sep 22, 2023

Just wanted to test PR builds at rtd

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.

2 participants