Skip to content

Polish documentation #77

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 27 commits into
base: main
Choose a base branch
from
Open

Polish documentation #77

wants to merge 27 commits into from

Conversation

clpetix
Copy link
Contributor

@clpetix clpetix commented Apr 14, 2025

This PR should fully address Issue #9.

Copy link
Contributor

Choose a reason for hiding this comment

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

Although lammpsio is broken up into separate Python files, the documented API all runs directly through the main namespace, like lammpsio.Box, and there aren't really that many different classes. Hence, I would recommend reorganizing all this into one "API" page (that you link into the main toc, no need to separate a User Guide and API Reference). Then, use autosummary with the toctree directive to autogenerate the documentation for our classes. You can check relentless for an example of doing this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would we handle the Topology module? We don't bring that in to the main namespace.

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 I would make a separate section & autosummary for topology, and refer to the classes by their fully qualified names to make that clear (lammpsio.topology.Bonds, etc.)

@@ -50,6 +65,18 @@ def cast(cls, value):
:class:`Box`
A simulation box matching the array.

Examples
Copy link
Contributor

Choose a reason for hiding this comment

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

This is great, I'd like to see more examples like this and have them run as unit tests with sybil.

Additionally, review all documentation for proper cross-referencing of classes, attributes, etc. I was probably rather lazy / inconsistent in doing so. Generally, single ticks get a cross reference, but double ticks make a verbatim code.

You may be able to also omit use of :class: everywhere to make this cross referencing simpler. I think that is not necessarily needed in modern sphinx.

@clpetix
Copy link
Contributor Author

clpetix commented Apr 17, 2025

@mphoward, one thing that we need to do is enable a discussion page in the repository for users to reach out if they need help. Can you do that when you have some time?

@mphoward
Copy link
Contributor

These should be enabled now!

@clpetix
Copy link
Contributor Author

clpetix commented May 20, 2025

@mayukh33, sybil will need to be added to the requirements.txt file in tests. That should fix the errors.

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.

3 participants