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

Overall doc review #15

Merged
merged 18 commits into from
Jan 12, 2024
Merged

Overall doc review #15

merged 18 commits into from
Jan 12, 2024

Conversation

PipKat
Copy link
Member

@PipKat PipKat commented Jan 9, 2024

General edits to RST files include using first person, active voice, and present tense when possible.

@tmpbeing, @Revathyvenugopal162, @jorgepiloto The acceptance and merging of this PR completes my overall doc review.
However, some observations follow. These can be moved into one or more issues and addressed in later PRs.

  • The doc does not contain the recommended five sections (Getting started, User guide, Examples, API reference, and Contribute. It has only two sections: User guide and API reference. Consider using the five recommended sections as you grow this project.
  • The landing pages for both the existing "User guide" and "API reference" sections are simply the toctree structures. Generally, landing pages provide introductory text and allow the left navigation pane provide for accessing the content of interest.
  • The "API reference" section uses Google style docstrings. All other (or at least the vast majority) of PyAnsys libraries use NumPy docstrings. This makes it different than other libraries and may make it harder for users to find what they need.
  • In the "User guide" section:
    • On the "Client configuration" page (generated from configuration.rst), there are descriptions for configuration and credential options. In some cases, there are two descriptions for an option. I've edited description in the PY files, but I don't know where the unedited descriptions are coming from. I searched the repository for these descriptions and found no matches. Is there a way to eliminate the duplicate descriptions? Also, there are no descriptions for the classmethod clean_url(url) configuration option or the classmethod prompt(values) credential option.
    • On the "Training" page (generated from training.rst), this is the sentence following the steps for training on prediction data: "Once you have training data in your project, you can use the web app to train a model." There should be a "For more information, see X." added here, with X being a link to information on the web app.
    • On the "Best practices" page (generated from best_practices.rst), there is only one topic. I assume you intend to add other topics over time. If not, perhaps change the name to "Asynchronicity" so that the title is more meaningful? Or, consider changing the title to this until more topics are available?
  • In the "API reference" section, the Postprocessings page has occurrences of SurfaceEvol. There is no English explanation of what this code entity is. For example, the description for the surface_eval method says: "Compute or get the SurfaceEvol for specific parameters." I didn't want to guess what "SurfaceEvol" was (although it is likely surface evolution). However, you should use plain English here in this docstring and in some others.

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Jan 9, 2024
Copy link
Collaborator

@tmpbeing tmpbeing left a comment

Choose a reason for hiding this comment

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

Thanks !

doc/source/api_reference/geometries.rst Outdated Show resolved Hide resolved
doc/source/api_reference/post_processings.rst Show resolved Hide resolved
doc/source/api_reference/predictions.rst Outdated Show resolved Hide resolved
doc/source/api_reference/predictions.rst Outdated Show resolved Hide resolved
doc/source/api_reference/training_data_parts.rst Outdated Show resolved Hide resolved
doc/source/api_reference/training_data.rst Outdated Show resolved Hide resolved
doc/source/api_reference/training_data_parts.rst Outdated Show resolved Hide resolved
doc/source/api_reference/workspaces.rst Outdated Show resolved Hide resolved
doc/source/user_guide/training.rst Outdated Show resolved Hide resolved
doc/source/user_guide/training.rst Outdated Show resolved Hide resolved
@PipKat PipKat marked this pull request as ready for review January 11, 2024 20:45
Copy link
Collaborator

@tmpbeing tmpbeing left a comment

Choose a reason for hiding this comment

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

Awesome work, thanks ! Just some very minor comments.

As for your suggestion. Some of them are already in our backlog, the others I'll bring to our product team.

doc/source/index.rst Outdated Show resolved Hide resolved
src/ansys/simai/core/api/post_processing.py Outdated Show resolved Hide resolved
src/ansys/simai/core/client.py Outdated Show resolved Hide resolved
src/ansys/simai/core/client.py Outdated Show resolved Hide resolved
src/ansys/simai/core/client.py Outdated Show resolved Hide resolved
src/ansys/simai/core/data/optimizations.py Outdated Show resolved Hide resolved
Copy link
Contributor

@Revathyvenugopal162 Revathyvenugopal162 left a comment

Choose a reason for hiding this comment

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

Looks fantastic as always!

src/ansys/simai/core/api/geometry.py Outdated Show resolved Hide resolved
src/ansys/simai/core/api/mixin.py Outdated Show resolved Hide resolved
src/ansys/simai/core/client.py Outdated Show resolved Hide resolved
src/ansys/simai/core/data/base.py Outdated Show resolved Hide resolved
src/ansys/simai/core/data/lists.py Outdated Show resolved Hide resolved
src/ansys/simai/core/data/predictions.py Outdated Show resolved Hide resolved
src/ansys/simai/core/utils/sse_client.py Outdated Show resolved Hide resolved
Incorporate feedback. Great catches all!

Co-authored-by: Revathy Venugopal <[email protected]>
Co-authored-by: Marc Planelles <[email protected]>
@tmpbeing tmpbeing merged commit c007955 into main Jan 12, 2024
30 checks passed
@tmpbeing tmpbeing deleted the doc/overall_review branch January 12, 2024 15:42
@tmpbeing
Copy link
Collaborator

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants