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

Move methods into class pages for docs #563

Merged
merged 1 commit into from
Oct 13, 2023

Conversation

arnaucasau
Copy link
Contributor

Summary

Moved all the methods and attributes into class pages to speed up the building process of the documentation with the new ecosystem sphinx theme. This change helps with this PR.

See Qiskit/qiskit#10455 for more information.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 6484045873

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage remained the same at 93.028%

Files with Coverage Reduction New Missed Lines %
qiskit_optimization/algorithms/goemans_williamson_optimizer.py 1 92.39%
Totals Coverage Status
Change from base Build 6249401371: 0.0%
Covered Lines: 4510
Relevant Lines: 4848

💛 - Coveralls

@arnaucasau arnaucasau marked this pull request as ready for review October 11, 2023 14:57
@woodsp-ibm
Copy link
Member

woodsp-ibm commented Oct 11, 2023

Now #562 is not yet merged - was the intent to do this first. I noted you opened a similar PR to #562 on qiskit-finance qiskit-community/qiskit-finance#302 but that is not draft. Maybe with the new theme things would look better here. Although personally having the lists/indices of attributes and functions with the one liner makes it easy to browse things quickly to find something you might be looking for. Now I would have to to scroll the entire page (and to me the functions do not stand out so well given they and the parameters are all highlighted similarly) - or glean enough info from the name in the right sidebar to perhaps go look at that. And speaking of right sidebar - why is the width of the page not take up fully e.g. in this screenshot

image

I see its the same with Qiskit - though at least it shows a scollbar there probably as it using the new theme whereas the docs build from this CI do not yet. Even so though it seems to behave the same and does not use extra space that is there which could allow the wrapped names to be on one line instead of having to wrap them which is made worse by all being prefixed by the class name.

@Eric-Arellano
Copy link
Contributor

Hey @woodsp-ibm

Now #562 is not yet merged - was the intent to do this first.

This PR is pre-work for #562. Without this PR, switching to the new Ecosystem Theme is prohibitively slow because Furo has O(n^2) time complexity for docs build time, with n being number of pages. This was explored in Qiskit/qiskit_sphinx_theme#328, including with the Furo maintainer.

The intent is to merge both this PR and #562. Arnau was only splitting this up into a dedicated PR so that code review is simpler. If your team prefers, they could be combined into one PR.

For other projects, n is small enough in the O(n^2) that switching to the new theme does not require this API docs reorganization. That's why some PRs aren't in draft mode.

Maybe with the new theme things would look better here.

Yeah, we had the same experience when switching Qiskit itself. The new theme makes the API pages much better, e.g. https://qiskit.org/documentation/stubs/qiskit.circuit.QuantumCircuit.html#qiskit.circuit.QuantumCircuit. The right side bar works better, and we worked with the design team to improve the design.

which is made worse by all being prefixed by the class name.

Unfortunately there's no fix for that to my knowledge.

@woodsp-ibm
Copy link
Member

woodsp-ibm commented Oct 12, 2023

@Eric-Arellano I am ok with the PRs as-is to save messing around, though it makes it harder to see what the final outcome is going to be since this is still using the older theme, hence the comments. But I guess we can bear with it now and we will see in the theme change PR, after this is merged and that updated, how it all looks. And if anything is needed further sort it then

As to the right hand sidebar. Things do not extend across the page in the API ref - at least what is published with Qiskit though perhaps that's not off the very latest theme either. E.g. this from QuantumCircuit below where you can see the menu bar above extends to the width of the browser but the API ref. contents do not, so the right hand bar, and you can see the scrollbar for it, has a bunch of white space to the right that if used would help allieviate the method name wrapping.

image

I think this sidebar will become more important to getting an overview of the class as the table, which provided that before and arguably better usability in that regard, is not longer created. I pretty much ignored that sidebar before but it seems like the only option now oing forwards if I want to quickly scope methods to narrow in on something that might do for the task I have in mind - aside from scrolling the entire page content. The sidebar is not as good imho in regard of providing such an overview as the prior table. since the latter had the one liner description in there too. But I guess we can see how it goes...

For other projects, n is small enough in the O(n^2) that switching to the new theme does not require this API docs reorganization.

But I assume this same will be done, e.g. Qiskit Finance, so the pages are all similar and all class methods on the one page for consistency for users - even if it does not have the same build time explosion.

@Eric-Arellano
Copy link
Contributor

I am ok with the PRs as-is to save messing around, though it makes it harder to see what the final outcome is going to be since this is still using the older theme, hence the comments.

Sg. I'm sure Arnau would be glad to update the Ecosystem Theme PR tomorrow to be built on top of this one so that you can get the final preview :)

Things do not extend across the page in the API ref

Yeah, that page layout was inherited by Furo. I spent some time this summer investigating changing the right side bar to always float to the right, but it was non-trivial so we decided to not prioritize it. Definitely doable, only a question of time. Tracked by Qiskit/qiskit_sphinx_theme#394. To set clear expectations, it's unlikely I'd have time to pick up that improvement since focus is on the new docs at https://docs.quantum-computing.ibm.com

@woodsp-ibm
Copy link
Member

...changing the right side bar to always float to the right,

I was more thinking of it being variable width, beyond some min value, so it took up that extra space and hopefully helped avoid some of the wrapping of the names it contained.

Copy link
Member

@woodsp-ibm woodsp-ibm left a comment

Choose a reason for hiding this comment

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

Approving based on above discussion.

@Eric-Arellano
Copy link
Contributor

I was more thinking of it being variable width, beyond some min value, so it took up that extra space and hopefully helped avoid some of the wrapping of the names it contained.

Ah, Furo's design is for it to always be the same width, even on mobile view where it uses a slideout menu.

Approving based on above discussion.

Thanks, Steve! Arnau doesn't have write permissions so this will need a merge from a maintainer.

@mergify mergify bot merged commit cddfbbb into qiskit-community:main Oct 13, 2023
15 checks passed
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