Skip to content

Conversation

MarcelKoch
Copy link
Member

This PR splits the documentation into a part that is generated by sphinx and one that is generated by doxygen.
The doxygen part will cover only the reference API documentation, and anything else will be left to sphinx. The full separation is not done in this PR.

Changes:

  • adds sphinx configuration
  • make doxygen available for sphinx
  • simplify doxygen setup
    • only the usr doc is available, the dev doc is removed. IMO there is no need for a dev doc, since we can expect developers to look into the Ginkgo source code and discover the documentation there.
  • fix some existing documentation (grouping, markdown files)
  • use read-the-docs instead of GH pages to deploy documentation

This is a continuation from #1679. Compared to the previous PR, this has a cleaner integration of doxygen into the documentation build by sphinx.

Link to generated docs: https://ginkgo-test.readthedocs.io/doxysphinx/index.html

Todo:

  • move to proper RTD space

@MarcelKoch MarcelKoch added reg:documentation This is related to documentation. 1:ST:ready-for-review This PR is ready for review labels Mar 20, 2025
@MarcelKoch MarcelKoch self-assigned this Mar 20, 2025
@ginkgo-bot ginkgo-bot added reg:build This is related to the build system. reg:ci-cd This is related to the continuous integration system. mod:core This is related to the core module. type:solver This is related to the solvers type:preconditioner This is related to the preconditioners type:matrix-format This is related to the Matrix formats type:factorization This is related to the Factorizations labels Mar 20, 2025
@MarcelKoch MarcelKoch mentioned this pull request Mar 20, 2025
1 task
MarcelKoch and others added 13 commits March 20, 2025 17:48
The linop group is often added with other groups. These other groups are mostly subgroups of linop, so there is no need to specify it twice.

Signed-off-by: Marcel Koch <marcel.koch@kit.edu>
This does the following things:
- add sphinx to create non-reference/api documentation
- move doxygen stuff into a subdirectory of the sphinx configuration
- enable read-the-docs to host documentation

Signed-off-by: Marcel Koch <marcel.koch@kit.edu>
Signed-off-by: Marcel Koch <marcel.koch@kit.edu>
Signed-off-by: Marcel Koch <marcel.koch@kit.edu>
Signed-off-by: Marcel Koch <marcel.koch@kit.edu>
Signed-off-by: Marcel Koch <marcel.koch@kit.edu>
Signed-off-by: Marcel Koch <marcel.koch@kit.edu>
- spelling
- fix yaml formatting

Co-authored-by: Thomas Grützmacher <thomas.gruetzmacher@tum.de>
@yhmtsai
Copy link
Member

yhmtsai commented Mar 24, 2025

Plain program under the example shows empty block.
https://ginkgo-test.readthedocs.io/doxysphinx/doxygen/html/simple_solver.html

Copy link
Member

@yhmtsai yhmtsai left a comment

Choose a reason for hiding this comment

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

I do not review the major part yet


{#
Copied from: https://github.com/executablebooks/sphinx-book-theme/blob/master/src/sphinx_book_theme/theme/sphinx_book_theme/macros/buttons.html
to remove the light/dark theme switcher button
Copy link
Member

Choose a reason for hiding this comment

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

why to remove light/data theme button?

Copy link
Member Author

Choose a reason for hiding this comment

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

because the doxygen API doesn't work with it. The doxygen thing will not change entirely, so there are always some blocks which are light colored.

Copy link
Member

Choose a reason for hiding this comment

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

I see. I am using dark theme. It hurts my eyes in some api pages like the following. I will not ask the changes about the style in this PR, but this will be definitely changed by setting background/font-color always or finding a color theme can fit dark/light theme well
image

Copy link
Member Author

Choose a reason for hiding this comment

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

This image is exactly the issue I mentioned. I have no idea how to fix those white blocks, so I would put this as an issue to fix later.

Copy link
Member

@pratikvn pratikvn Mar 31, 2025

Choose a reason for hiding this comment

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

Maybe we can change the Sphinx theme ? I prefer ones where both dark and light themes work well. Some themes which seem to be quite nice:

  1. https://sphinx-book-theme.readthedocs.io/en/stable/
  2. https://pydata-sphinx-theme.readthedocs.io/en/stable/index.html

In particular, the PyData seems nice, because it has support for Jupyter extensions, which could be useful for us.

Copy link
Member Author

Choose a reason for hiding this comment

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

The current theme is sphinx-book. Because of doxysphinx, we are limited to the sphinx themes supported by them: https://boschglobal.github.io/doxysphinx/#caveats.
The issue with the light/dark theme isn't due to the sphinx theme, it's due to doxygen. I'm sure it's possible to get it working, but I don't have it on my priority list.

Copy link
Member

Choose a reason for hiding this comment

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

Honestly, I am not sure how useful the detailed doxygen documentation is. It might be better to not have the complete class/namespace list, but only focus on specific user-facing API and integrate it in the Sphinx documentation through breathe: https://breathe.readthedocs.io/en/latest/

Copy link
Member Author

Choose a reason for hiding this comment

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

Breathe makes the doxygen API available as sphinx docs, which has the advantage of simpler integration, but the generated formatting of C++ API is just plain horrible. Since Sphinx is python focus it doesn't care about parameter types, so types aren't aligned for multi-line function signatures.

@@ -1,4 +1,4 @@
// SPDX-FileCopyrightText: 2017 - 2024 The Ginkgo authors
// SPDX-FileCopyrightText: 2017 - 2025 The Ginkgo authors
Copy link
Member

Choose a reason for hiding this comment

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

should jacobi.hpp be deleted in general?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know, this just keeps all of the documentation as it currently is. This PR changes only how it's presented.

MarcelKoch and others added 3 commits March 24, 2025 14:12
- deactivate unnecessary cmake options
- typos
- generate sphinx config info

Co-authored-by: Yu-Hsiang M. Tsai <yhmtsai@gmail.com>
@MarcelKoch
Copy link
Member Author

Plain program under the example shows empty block. https://ginkgo-test.readthedocs.io/doxysphinx/doxygen/html/simple_solver.html

Should be fixed now.

Copy link
Member

@yhmtsai yhmtsai left a comment

Choose a reason for hiding this comment

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

need to remove something like GINKGO_DOC_GENERATE_DEV in install.md


{#
Copied from: https://github.com/executablebooks/sphinx-book-theme/blob/master/src/sphinx_book_theme/theme/sphinx_book_theme/macros/buttons.html
to remove the light/dark theme switcher button
Copy link
Member

Choose a reason for hiding this comment

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

I see. I am using dark theme. It hurts my eyes in some api pages like the following. I will not ask the changes about the style in this PR, but this will be definitely changed by setting background/font-color always or finding a color theme can fit dark/light theme well
image

add_custom_command(TARGET "doxygen" POST_BUILD
COMMAND make
COMMAND "${CMAKE_COMMAND}" -E copy refman.pdf
"${CMAKE_CURRENT_BINARY_DIR}/${name}.pdf"
Copy link
Member

Choose a reason for hiding this comment

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

name is undefined here

CREATE_LINK ${Ginkgo_SOURCE_DIR}/LICENSE ${Ginkgo_BINARY_DIR}/LICENSE
SYMBOLIC
)
file(
Copy link
Member

Choose a reason for hiding this comment

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

do we use test_install somewhere in documentation?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, in install.md, right at the bottom.

Copy link
Member

Choose a reason for hiding this comment

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

I have tested it, but it actually downloaded the file.
I think it should be a link to the github page on the code rather than download that file.

- copy directories directly
- add missing examples
- remove doc of removed cmake option

Co-authored-by: Yu-Hsiang M. Tsai <yhmtsai@gmail.com>
@@ -0,0 +1,38 @@
# See https://docs.readthedocs.io/en/stable/config-file/v2.html for details
Copy link
Member

@yhmtsai yhmtsai Mar 25, 2025

Choose a reason for hiding this comment

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

after this pr, we need to have registered in readthedoc. They will constantly fetch the repo or get the notification by some API hook. We do not need to do anything on our CI, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it gets automatically picked up: MarcelKoch#20.
I think all of the configuration happens on the read-the-docs side.

Copy link
Member

Choose a reason for hiding this comment

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

I see. I have also seen https://docs.readthedocs.com/platform/stable/guides/automation-rules.html such that we can set up a rule to just make the release and latest version documentation not-hidden to user.
We can make the normal branch either in hidden state (still build) or deactivated (no build).

@yhmtsai
Copy link
Member

yhmtsai commented Mar 25, 2025

for code ```c++ will give a ++ in the beginning on the section https://ginkgo-test.readthedocs.io/doxysphinx/doxygen/html/group__Executor.html
image

@MarcelKoch
Copy link
Member Author

for code ```c++ will give a ++ in the beginning on the section

It's the same for our current documentation: https://ginkgo-project.github.io/ginkgo-generated-documentation/doc/develop/group__Executor.html#gae20be793db14dfd6249a315281201550.
I want to keep changes to the actual text of the documentation out of this PR, so I would not fix this right now.

- use link to GH repo instead of relative file path

Co-authored-by: Yu-Hsiang M. Tsai <yhmtsai@gmail.com>
@ginkgo-bot
Copy link
Member

Error: The following files need to be formatted:

core/solver/ir.cpp
include/ginkgo/core/multigrid/pgm.hpp

You can find a formatting patch under Artifacts here or run format! if you have write access to Ginkgo

Copy link
Member

@yhmtsai yhmtsai left a comment

Choose a reason for hiding this comment

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

Thanks for adding this documentation configuration.
You seems to still update something. I will review again when you finish.

@@ -0,0 +1,38 @@
# See https://docs.readthedocs.io/en/stable/config-file/v2.html for details
Copy link
Member

Choose a reason for hiding this comment

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

I see. I have also seen https://docs.readthedocs.com/platform/stable/guides/automation-rules.html such that we can set up a rule to just make the release and latest version documentation not-hidden to user.
We can make the normal branch either in hidden state (still build) or deactivated (no build).

project = 'Ginkgo'
copyright = f'{datetime.date.today().year}, The Ginkgo Authors'
author = 'The Ginkgo Authors'
release = '@Ginkgo_VERSION@'
Copy link
Member

Choose a reason for hiding this comment

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

maybe need the version tag to distinguish develop/release?

@MarcelKoch MarcelKoch marked this pull request as draft May 6, 2025 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1:ST:ready-for-review This PR is ready for review mod:core This is related to the core module. reg:build This is related to the build system. reg:ci-cd This is related to the continuous integration system. reg:documentation This is related to documentation. type:factorization This is related to the Factorizations type:matrix-format This is related to the Matrix formats type:preconditioner This is related to the preconditioners type:solver This is related to the solvers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants