-
Notifications
You must be signed in to change notification settings - Fork 955
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.
Thanks for this. I left one small suggestion.
Reviewed 19 of 19 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @kgryte)
src/util.ts, line 45 at r1 (raw file):
} /** Returns a sample from a uniform [a, b) distribution. */
Rather than just change the final bracket to a paren, could you add a second line to the comment indicating that the range includes 'a' but excludes 'b'.
@tafsiri Updated the PR to satisfy the requested change. Thanks for reviewing! |
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.
Thank you!
Reviewed 18 of 19 files at r1, 1 of 1 files at r2.
Reviewable status: 0 of 1 approvals obtained (waiting on @kgryte)
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.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @kgryte)
src/ops/binary_ops.ts, line 102 at r2 (raw file):
util.assert( Array.isArray(tensors), () => 'The argument passed to tf.addN() must be a list of tensors');
changing this message requires a change in a unit test. See log: https://travis-ci.org/tensorflow/tfjs-core/jobs/420267519#L531
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.
Reviewable status: 0 of 1 approvals obtained (waiting on @kgryte)
src/ops/binary_ops.ts, line 102 at r2 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
changing this message requires a change in a unit test. See log: https://travis-ci.org/tensorflow/tfjs-core/jobs/420267519#L531
Tip: Run yarn test
on your local machine to quickly detect broken unit tests
@dsmilkov Addressed. Sorry for not catching this. I had naively assumed that updating an error message would not affect tests. As an aside, is the project guaranteeing API stability of error messages? Or from a user's POV, should I expect that error messages are fixed? Because if so, I might suggest standardizing error messages and possibly consider other alternatives which might also allow library consumers to programmatically identify errors (which I presume is why error messages are tested against). One such approach is using error codes, as found in Node.js and in HTTP headers. At least for me, was not obvious that fixing a typo in an error message would be a breaking API/behavior change. |
No guarantee for API stability of error messages yet. We test for those error messages to make sure that the user gets an informative error message, since it is not uncommon for an assert statement to fail before your expected assert fails, resulting in a non-helpful message from the former assert. |
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.
Reviewed 1 of 1 files at r3.
Reviewable status: 0 of 1 approvals obtained (waiting on @kgryte)
Description
This PR seeks to address a random collection of typos and formatting irregularities. No behavioral changes should be introduced.
For repository owners only:
Please remember to apply all applicable tags to your pull request.
Tags: FEATURE, BREAKING, BUG, PERF, DEV, DOC, SECURITY
For more info see: https://github.com/tensorflow/tfjs/blob/master/DEVELOPMENT.md
This change is