-
Notifications
You must be signed in to change notification settings - Fork 79
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
Standardizing commenting style for major functions #799
Standardizing commenting style for major functions #799
Conversation
…menting-style' into 798-standardize-commenting-style
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
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.
Seems like the comments have been added on some of the most important portions of the code, which is great start!
It would be good to expand this for metrics and losses as well and include that in this PR.
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.
Hey there, thanks for the updates! I have commented on just few things regarding the typings for List type in few places (they maybe were skipped on purpose, if so it should be left as that).
I am very happy with the way this PR has been worked out. Special thanks to the contributions of @Geeks-Sid and @szmazurek for helping review this 🦣 PR. 😄 |
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.
LGTM
Fixes #798
Proposed Changes
typing
related updates for readabilitytuple
-->Tuple
list
-->List
Optional
Checklist
CONTRIBUTING
guide.pip install
step is needed for PR to be functional), please ensure it is reflected in all the files that control the CI, namely: python-test.yml, and all docker files [1,2,3].