-
-
Notifications
You must be signed in to change notification settings - Fork 173
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
C++ demos document generation #3052
Conversation
I think we would like to keep the single source file per demo approach. Jupytext, the tool we use to convert light formatted Python to myst flavoured markdown, also supports converting light formatted C++ to myst flavoured markdown, which can then be included in sphinx using thr myst-parser module. |
Additionally, this will require converting the existing rst comments to myst flavoured markdown. There is an automated tool to do this. https://docs.readthedocs.io/en/stable/guides/migrate-rest-myst.html |
Thanks for looking at this. We aim with the doc system to be as 'standard' and simple as possible, especially at the lowest levels in the processing chain. We've seen (many times over now!) that we're not able to maintain bespoke doc processing. Ideally on the C++ we can just run |
@garth-wells I'm having a chat with @ampdes in Slack #development, I think we've found a good solution; for the demos we use light formatted C++ + myst-flavoured markdown + jupytext + sphinx/breathe, just like for the Python demos. It's been very robust so far in Python. A link to the C++ demos will appear here: https://docs.fenicsproject.org/dolfinx/v0.7.3/cpp/ Everything else stays the same, including the standard doxygen build for API documentation and the CI steps. |
This is not yet configured to run in github workflows. |
cpp/demo/hyperelasticity/main.cpp
Outdated
// display_name: C++17 | ||
// language: C++17 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do C++20 here? That is the minimal required standard we have for the actual code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifying kernel_spec is not necessary. This is only needed when this file is opened in a notebook environment, then kernel_spec will be used for a kernel with interactive compiler.
I was trying xeus-cling locally (and not in dolfinx docker containers) and this jupytext metadata is a remant from that explore.
One could have just got hold of main.cpp
file and run
~/.local/bin/jupytext --set-formats cpp,md main.cpp
to bind .cpp
with a .md
file, which results in
---
jupyter:
jupytext:
cell_metadata_filter: -all
formats: cpp,md
main_language: c++
text_representation:
extension: .md
format_name: markdown
format_version: '1.3'
jupytext_version: 1.16.1
---
in .md
file and
// ---
// jupyter:
// jupytext:
// cell_metadata_filter: -all
// formats: cpp:light,md
// text_representation:
// extension: .cpp
// format_name: light
// format_version: '1.5'
// jupytext_version: 1.16.1
// ---
.cpp
file.
https://jupytext.readthedocs.io/en/latest/advanced-options.html
I will simply remove all kernel_spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually only
~/.local/bin/jupytext --set-formats cpp main.cpp
is needed for each main.cpp
file to populate with jupytext header, which will then be used later to convert to .md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed this discussion; does xeus-cling work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was presuming that this jupytext header is needed for reading and then converting to .md
files.
It turns out that all these specifications are only for connecting .md
or .cpp
files to notebook files.
All these are deleted.
I didn't yet try xeus-cling with dolfinx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, so perhaps we can also reduce or remove them in the .py files.
* Rename README to README.md in demo * Fix typos in doc of custom_kernel * Add basic documentation for hyperelasticity
* create a CMakeLists for documentation * supply INPUT and OUTPUT DIRS in Doxygen from Cmake * add FindSphinx * restructure source/index.rst to include demo.rst * api.rst was previously index.rst
* include flags for code blocks in demo cpp file(s) * create demo explanation as rst in /doc dir * refer code snippets in demo rst file
* add jupyter_process for cpp demo * convert biharmonic demo doc to myst * index demo.rst * copy python conf.py to cpp docs
* Add problematic files back
* C++ demos are organized in directory
* add jupytext specifications * update jupytext_process to include all demos
* delete CMakeLists.txt * revert Doxyfile * revert conf.py
bc46e5a
to
fee2eb3
Compare
with open(myst_file, "w") as fw: | ||
fw.write(cpp_myst_text) | ||
|
||
# There is a posibility to use jupyter-notebooks with C++/C kernels |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel C++ and Notebooks is a bit of an edge use case.
However, free to export them, but don't link to them at this stage; someone could take a look and see if https://github.com/jupyter-xeus/xeus-cling works with the conda FEniCS packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still a few more things to do; looking very good.
cpp/demo/biharmonic/main.cpp
Outdated
// .. math:: | ||
// u &= 0 \quad {\rm on} \ \partial\Omega \\ | ||
// \nabla^{2} u &= 0 \quad {\rm on} \ \partial\Omega | ||
// $$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be better to use the amsmath environments rather than $$
https://myst-parser.readthedocs.io/en/v0.14.0/using/syntax-optional.html#syntax-amsmath
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One could use both $$ and amsmath simultaneously.
cpp/doc/Doxyfile
Outdated
../dolfinx/common/loguru.cpp | ||
../dolfinx/common/loguru.cpp \ | ||
# TODO: resolve Doxygen parsing in: | ||
# ../dolfinx/fem/interpolate.h \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please discuss in Slack if struggling.
cpp/demo/biharmonic/main.cpp
Outdated
// --- | ||
// jupyter: | ||
// jupytext: | ||
// cell_metadata_filter: -all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out it isnt needed unless for connecting the scripts to notebooks.
a55eb23
to
0c61ebe
Compare
This looks close to being ready to merge? |
I'm going to merge this, cleanup and tweaking can be done later. |
@ampdes very nice job. |
An illustration for one of the demos: biharmonic equation