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

Docstrings for cuda.core #186

Merged
merged 13 commits into from
Nov 7, 2024
Merged

Docstrings for cuda.core #186

merged 13 commits into from
Nov 7, 2024

Conversation

vzhurba01
Copy link
Collaborator

@vzhurba01 vzhurba01 commented Oct 23, 2024

Use NumPy Style Python Docstrings to maintain consistency with cuda-bindings.

Edit:
Close #96
Close #207

@vzhurba01 vzhurba01 requested a review from leofang October 23, 2024 21:08
@vzhurba01 vzhurba01 self-assigned this Oct 23, 2024
Copy link
Contributor

copy-pr-bot bot commented Oct 23, 2024

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@vzhurba01 vzhurba01 changed the title Docstrings for cuda.core Device class Docstrings for cuda.core Oct 24, 2024
@leofang leofang requested a review from ksimpson-work October 25, 2024 19:11
@leofang leofang added documentation Improvements or additions to documentation cuda.core Everything related to the cuda.core module P0 High priority - Must do! P1 Medium priority - Should do and removed P0 High priority - Must do! labels Oct 25, 2024
@leofang leofang added this to the cuda.core beta 1 milestone Oct 25, 2024
@vzhurba01
Copy link
Collaborator Author

This PR now has the remaining items from issue #96 documented and is ready for review. I've updated description to automatically close out the issue upon submission.

ksimpson-work
ksimpson-work previously approved these changes Oct 30, 2024
Copy link
Contributor

@ksimpson-work ksimpson-work left a comment

Choose a reason for hiding this comment

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

I'm a fan. I added some super minor nits that you can choose to ignore.

ksimpson-work
ksimpson-work previously approved these changes Nov 1, 2024
Copy link
Member

@leofang leofang left a comment

Choose a reason for hiding this comment

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

LGTM overall, thanks a ton for the careful documentation effort Vlad! I took a few quick glances.

One general comment: For classes, let's not document __init__ (and certainly not __new__, which is an implementation detail). We merge everything from the constructor to the class docstring, otherwise Sphinx might be confused:

Either form is acceptable, but the two should not be mixed. Choose one
convention to document the __init__ method and be consistent with it.

https://www.sphinx-doc.org/en/master/usage/extensions/example_numpy.html

Below is one example how it would look like (note that we need to manually copy the constructor signature on the first line):
https://github.com/NVIDIA/nvmath-python/blob/7c485842d0f3300e03ec780056936503913910fe/nvmath/fft/fft.py#L537-L541

@vzhurba01
Copy link
Collaborator Author

LGTM overall, thanks a ton for the careful documentation effort Vlad! I took a few quick glances.

One general comment: For classes, let's not document __init__ (and certainly not __new__, which is an implementation detail). We merge everything from the constructor to the class docstring, otherwise Sphinx might be confused:

Either form is acceptable, but the two should not be mixed. Choose one
convention to document the __init__ method and be consistent with it.

https://www.sphinx-doc.org/en/master/usage/extensions/example_numpy.html

Below is one example how it would look like (note that we need to manually copy the constructor signature on the first line): https://github.com/NVIDIA/nvmath-python/blob/7c485842d0f3300e03ec780056936503913910fe/nvmath/fft/fft.py#L537-L541

Done

Copy link
Member

@leofang leofang left a comment

Choose a reason for hiding this comment

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

Thanks, Vlad, LGTM!

I pushed commit 7e25688 to bring the docs up-to-date. @vzhurba01 could you try to build locally and see if you encounter any issues? If not, feel free to merge!

@vzhurba01 vzhurba01 merged commit 72acaaf into NVIDIA:main Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cuda.core Everything related to the cuda.core module documentation Improvements or additions to documentation P1 Medium priority - Should do
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add known issues to v0.1.0 release note Complete docstrings for public APIs
3 participants