-
Notifications
You must be signed in to change notification settings - Fork 80
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
documentation: move num_chunks doc comment in ConvertApplyOpPattern #3770
Conversation
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.
Minor style comments/questions
tensor sizes it handles. Higher values may increase compute overhead but reduce size of | ||
communication buffers when lowered. | ||
If there are several candidate prefetch ops, the one with the largest result buffer | ||
size is selected. |
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.
Do newlines get included in the docs or is it more like latex where single newlines are no-ops?
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.
The latter, same rule with double newline, you can also add them explicitly, I think it's basically markdown for the online doc rendering (and I think for VSCode hover)
""" | ||
|
||
num_chunks: int = 1 | ||
""" | ||
number of chunks into which communication and computation should be split. |
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.
Should we start doc comments with capital letters?
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.
Hmm, I would say yes, and should end in .
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 wanted to minimise the changes here but actually I'll make this one.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3770 +/- ##
=======================================
Coverage 91.25% 91.25%
=======================================
Files 461 461
Lines 57478 57479 +1
Branches 5543 5543
=======================================
+ Hits 52454 52455 +1
Misses 3602 3602
Partials 1422 1422 ☔ View full report in Codecov by Sentry. |
…dslproject#3770) Makes the doc comment more localized and is rendered better in the online doc (upcoming).
Makes the doc comment more localized and is rendered better in the online doc (upcoming).