[rocThrust][Code Coverage] Increase rocThrust code coverage#174
Conversation
NguyenNhuDi
requested review from
RobsonRLemos,
lawruble13,
stanleytsang-amd and
umfranzw
as code owners
…ncrease-code-coverage-rs-7.0
umfranzw
left a comment
umfranzw
left a comment
There was a problem hiding this comment.
I think this looks good - I just have a few minor comments.
|
|
||
| TESTS_PAIRS_DEFINE(ComplexPairsTests, PairsTestsParams) | ||
|
|
||
| TYPED_TEST(ComplexPairsTests, TestAsignOperator) |
There was a problem hiding this comment.
Just a little thing - the test name has a typo (should be TestAssignOperator (with a double 's').
There was a problem hiding this comment.
Thanks Wayne, I have updated this in my latest commit
| type bc = b * c; | ||
| type ad = a * d; | ||
| type denom = c * c + d * d; | ||
| type real = (ac + bd) / denom; |
There was a problem hiding this comment.
I noticed that the test values are randomly generated (in run_compound_tests()), in a range that spans zero. Do we need to watch out for division by zero here (lines 595, 596, 601, 604, 607)?
There was a problem hiding this comment.
Hi Wayne, in this case we do not have to worry about division by zero. This is because I'm not generating random numbers but instead iterating over a range.
In run_compound_tests I am taking the difference between maxi and mini, dividing it by 15 (so we have 15^4 different combination of numbers) and incrementing it that way.
If you take a look at line 443, 445, 447 abd 449 you can see this. While it spans over 0, it will never actually be 0.
| return thrust::detail::complex::log1pf(x); | ||
| }); | ||
| } | ||
| # endif // __HIP__ |
There was a problem hiding this comment.
The comment here doesn't look like it's correct - I'd suggest either removing the comment, or changing __HIP__ to the condition from line 224: !defined(__CUDACC__) && !defined(_NVHPC_CUDA)
|
|
||
| double denom = std::cosh(2.0 * t_real) + std::cos(2 * t_imag); | ||
|
|
||
| T real = std::sinh(2.0 * t_real) / denom; |
There was a problem hiding this comment.
Just wanted to check if we need to worry about division by zero here again (lines 1077, 1078), since it looks like the range (the mini and maxi arguments given on lines 1085, 1086) spans zero.
There was a problem hiding this comment.
Similar to the above. We are spanning over 0, getting near it but never actually having 0.
|
Closing this PR to rebase to develop instead. New PR here: #410 |
Remaking this PR #174 to merge into develop instead of release-staging branch --------- Co-authored-by: spolifroni-amd <Sandra.Polifroni@amd.com>
Remaking this PR ROCm#174 to merge into develop instead of release-staging branch --------- Co-authored-by: spolifroni-amd <Sandra.Polifroni@amd.com>
Remaking this PR #174 to merge into develop instead of release-staging branch --------- Co-authored-by: spolifroni-amd <Sandra.Polifroni@amd.com>
[ROCm/hipFFT commit: 4df4ddf]
* Rename frontend integration * Change layout casing * Revert "Change layout casing" This reverts commit c83b8ef6cb525679dcbb862953379bf7ac76916d. * Rename frontend integration * Format * Rename file
* Rename frontend integration * Change layout casing * Revert "Change layout casing" This reverts commit c83b8ef6cb525679dcbb862953379bf7ac76916d. * Rename frontend integration * Format * Rename file [ROCm/hipDNN commit: 08c90f5]
4 participants
This PR aims to increase the code coverage of rocThrust and to improve the accuracy of code coverage reports generated using llvm-cov.
To increase the accuracy of code coverage reports it is necessary to decouple the
test_header.hpp. For example, an unit test testing functions forbinary_searchshould not have access to the complex functions. If it does, due to the merging behavior of llvm-cov the generated report will have a lower score.To accomodate this
test_header.hpphas its contented divided into 3 new headers, and some functionality got migrated totest_utils.hpptest_param_fixtures.hppwill now contain all the templates and parameters necessary for typed test suites.test_real_assertions.hppwill now handle all the unit test assertions for real numbers and should not be included by complex unit tests.test_imag_assertions.hppwill now handle all the unit test assertions for complex numbers and should not be included by any unit tests except for complex ones.test_utils.hppinherited HIP_CHECK, test and inter_run_bwr namespace fromtest_header.hpp