Skip to content

docs: fix documentation rendering with scikit-package standards. #64

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

Open
wants to merge 2 commits into
base: migration
Choose a base branch
from

Conversation

ycexiao
Copy link

@ycexiao ycexiao commented Jul 30, 2025

What problem does this PR address?

Fix the documentation rendering.

What should the reviewer(s) do?

Please check the rendered documentation.
image
image
image
image

Copy link
Author

@ycexiao ycexiao left a comment

Choose a reason for hiding this comment

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

@sbillinge, it's ready for review.

@@ -1,3 +1,3 @@
{
"path": "../../../../examples/QPA-Quantitative phase analysis.ipynb"
Copy link
Author

Choose a reason for hiding this comment

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

Examples with .ipynb files are moved from the top-level directory into the docs directory, as other diffpy packages do.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think these should be in the top level tbh. These are examples for users, not source for docs so it doesn't make so much sense to me to put them into docs/source. It kind of depends whether the examples are just going to appear in the rst or whether we expect users to be able to import and run them. They won't get bundled in the release, but at least they are more accessible (in docs/examples in the repo)

Copy link
Contributor

Choose a reason for hiding this comment

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

wait, scratch my comment. As long as it is docs/examples I am happy...

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, actualy, thinking again, this change we may want to run by @vincefn to see his preference. I could also see examples at the top level making them more discoverable. I think if it were my choice I would do docs/examples but let's give @vincefn a chance to puah back before we merge this.

@@ -32,7 +32,7 @@ was developed by V. Favre-Nicolin as part of the development of the
Further developments including the ability to index and refine
powder patterns, solve and display crystal structures, using the
global optimisation and least squares algorithms (see the
:doc:`examples/index`) are provided by Vincent Favre-Nicolin (ESRF).
:doc:`examples/examples`) are provided by Vincent Favre-Nicolin (ESRF).
Copy link
Author

Choose a reason for hiding this comment

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

examples/index.rst is renamed to examples/examples.rst.

@ycexiao ycexiao marked this pull request as ready for review July 30, 2025 18:23
@ycexiao
Copy link
Author

ycexiao commented Jul 31, 2025

@sbillinge , is it ready for merging?

@vincefn
Copy link
Collaborator

vincefn commented Jul 31, 2025

Who should I add as maintainer on readthedocs, so you can also test the version generated there ?

@Tieqiong
Copy link

Are there any additional steps to install pyobjcrst?

@ycexiao
please run
conda install scons
and
scons -j4 dev

This will produce a _pyobjcryst.* in your src/pyobjcryst. Then generate api and build html again. You should be able to see the modules and C++ signatures

@ycexiao

This comment has been minimized.

@sbillinge
Copy link
Contributor

Who should I add as maintainer on readthedocs, so you can also test the version generated there ?

@vincefn please could you add me (@sbillinge )

@sbillinge
Copy link
Contributor

@vincefn quick question. Do you mind if we move Examples under docs (current standard in diffpy) or do you want to keep them at the top level (e.g., for easier discovery)? Ok to do it either way so just let us know.

@Tieqiong
Copy link

Tieqiong commented Aug 2, 2025

Could you please help me with the installation problem?

@ycexiao what's the status now? Maybe you can post your screenshot here for reference. Thanks

@ycexiao
Copy link
Author

ycexiao commented Aug 2, 2025

Could you please help me with the installation problem?

@ycexiao what's the status now? Maybe you can post your screenshot here for reference. Thanks

Thank you, the message is outdated. We have solved this through Slack.

@sbillinge
Copy link
Contributor

@ycexiao please could you summarize where we are with everything on this PR? I think, but not sure, that we have discussed:

  1. moving examples to docs/examples. Status: waiting to hear vincefn preferrence
  2. discussed building richer API docs with docstrings from the c-code. Status: not sure the status
  3. discussed whether to deploy docs to gh-pages or to readthedocs. Status: Status: waiting to hear vincefn preferrence
  4. not discussed but pertinent: whether to change the theme to pydata as per pyobjcryst current docs.

@ycexiao
Copy link
Author

ycexiao commented Aug 3, 2025

discussed building richer API docs with docstrings from the c-code. Status: not sure the status

@sbillinge
We can parse the C code via doxygen and create the C API using breathe, an extension for sphinx. It has a good rendering effect for well-documented classes and functions.

image

To add C API:

  1. Install doxygen and breathe.
  2. Edit Doxyfile, which is like conf.py to sphinx
  3. Run doxygen Doxyfile.
  4. Add breathe dependence and settings in conf.py
  5. Add links to the C files/class/functions in rst files.
  6. Run make html. And the C API page is included in the documentation.

We can modify Makefile so that we don't need to run doxygen manually. Please see if it is a good way to add C API pages. If it is, I will refine my current setups and commit them to this PR.

@Tieqiong
Copy link

Tieqiong commented Aug 3, 2025

@ycexiao we don't need these to render the c api. The current method is fine, just remember to generate your extension module locally and sphinx will do everything for you.
But if we want all the hyperlinks for c types, maybe this is the way to go?

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.

4 participants