-
Notifications
You must be signed in to change notification settings - Fork 365
feat(//tests): Update rtol and atol based tolerance for test cases #1055
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
Conversation
Signed-off-by: Anurag Dixit <a.dixit91@gmail.com>
Current threshold in test cases will act as absolute tolerance limit as intended. |
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.
Code conforms to Python style guidelines
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.
Code conforms to C++ style guidelines
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.
Code conforms to C++ style guidelines
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.
Code conforms to Python style guidelines
LOG_GRAPH(a << std::endl << b << std::endl); | ||
auto a_float = a.toType(at::kFloat); | ||
auto b_float = b.toType(at::kFloat); | ||
|
||
auto diff = a_float - b_float; | ||
auto result = diff.abs().max().item<float>() - (atol + rtol * b.abs().max().item<float>()); | ||
auto result = diff.abs().max().item<float>(); |
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.
This equation is still confusing (see #1052)
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.
Based on the discussion in 1052, is this resolved? Anurag mentioned we need this PR for DLFW release.
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.
Code conforms to Python style guidelines
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.
Code conforms to C++ style guidelines
Signed-off-by: Anurag Dixit <a.dixit91@gmail.com>
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.
Code conforms to Python style guidelines
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.
There are some changes that do not conform to C++ style guidelines:
diff --git a/workspace/tests/util/util.cpp b/tmp/changes.txt
index 6dc0d81..13d0d18 100644
--- a/workspace/tests/util/util.cpp
+++ b/tmp/changes.txt
@@ -18,7 +18,7 @@ bool almostEqual(const at::Tensor& computed_tensor, const at::Tensor& gt_tensor,
auto result = diff.abs().max().item<float>();
auto threshold = atol + (rtol * gt_tensor.abs().max().item<float>());
- LOG_GRAPH(std::string("Max Difference: ") + std::to_string(result) );
+ LOG_GRAPH(std::string("Max Difference: ") + std::to_string(result));
LOG_GRAPH(std::string("Acceptable Threshold: ") + std::to_string(threshold));
return result <= threshold;
ERROR: Some files do not conform to style guidelines
Signed-off-by: Anurag Dixit <a.dixit91@gmail.com>
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.
Code conforms to Python style guidelines
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.
Code conforms to C++ style guidelines
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 according to the equation that torch.allclose is using
LOG_GRAPH(a << std::endl << b << std::endl); | ||
auto a_float = a.toType(at::kFloat); | ||
auto b_float = b.toType(at::kFloat); | ||
|
||
auto diff = a_float - b_float; | ||
auto result = diff.abs().max().item<float>() - (atol + rtol * b.abs().max().item<float>()); | ||
auto result = diff.abs().max().item<float>(); |
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.
Based on the discussion in 1052, is this resolved? Anurag mentioned we need this PR for DLFW release.
Signed-off-by: Anurag Dixit a.dixit91@gmail.com
Description
Updating the tolerance metric using absolute and relative tolerance threshold.
This PR directly impacts the CI, DLFW workflow.
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (issue)
#1054
Type of change
Please delete options that are not relevant and/or add your own.
Checklist: