-
Notifications
You must be signed in to change notification settings - Fork 267
Add unit tests for template optimization helpers #3610
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
base: mpodkory/build-time-docs
Are you sure you want to change the base?
Add unit tests for template optimization helpers #3610
Conversation
1e1fac9 to
8d2b954
Compare
9b5a7fa to
867fae3
Compare
69144aa to
0b35651
Compare
Add Google Tests for optimized template utilities: - sequence_gen: Tests with custom functors (4 tests) - generate_identity_sequences: Tuple of identity sequences (4 tests) - find_in_tuple_of_sequences: O(1) sequence search (6 tests) - sequence_find_value: Value lookup in sequences (5 tests) - container_concat: Tuple/array concatenation (5 tests) - make_uniform_tuple: Repeated value tuples (4 tests) - compute_element_space_size: Fold expression (8 tests) - unpack_and_merge_sequences: Sequence merging (2 tests) Total: 43 new tests across 4 test files.
0b35651 to
0c81883
Compare
97983c9 to
ecef7c8
Compare
80c4f97 to
71413bd
Compare
| // element_space_size = 1 + (4-1)*4 + (4-1)*1 = 1 + 12 + 3 = 16 | ||
| constexpr auto lengths = make_tuple(Number<4>{}, Number<4>{}); | ||
| constexpr auto strides = make_tuple(Number<4>{}, Number<1>{}); | ||
| constexpr auto result = detail::compute_element_space_size(lengths, strides, Sequence<0, 1>{}); |
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.
Why are we testing an implementation detail?
| EXPECT_EQ(result, 5); | ||
| } | ||
|
|
||
| // Test make_naive_tensor_descriptor uses compute_element_space_size correctly |
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 is the right way to test the element space size.
| // Test make_naive_tensor_descriptor uses compute_element_space_size correctly | ||
| TEST(MakeNaiveTensorDescriptor, ElementSpaceSize2D) | ||
| { | ||
| constexpr auto lengths = make_tuple(Number<4>{}, Number<4>{}); |
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.
Can we avoid duplicate values unless we need to? Maybe use lengths 5, 4 with strides 4, 1?
| { | ||
| // 4x4 tensor with padded strides [8, 1] (8 elements per row instead of 4) | ||
| // element_space_size = 1 + (4-1)*8 + (4-1)*1 = 1 + 24 + 3 = 28 | ||
| constexpr auto lengths = make_tuple(Number<4>{}, Number<4>{}); |
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.
With padding we can make all the different (without common factors, maybe prime numbers), e.g., lengths 11, 7, 3 strides 2, 97, 23,
This way the integer math is more likely to catch bad index.
This example is a valid stride, but doesn't have a simple meaning (some tiling ops might not work, but it's a good edge case for testing the element space size).
| EXPECT_EQ(desc.GetElementSpaceSize(), 5); | ||
| } | ||
|
|
||
| // Test with runtime values (index_t instead of Number<>) |
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.
These are good runtime value tests, but again let's avoid square shapes.
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.
Also don't test implementation details.
shumway
left a comment
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 looks good, thanks! It looks like CI failed, so we'll have to recheck. My only feedback is for the element space size:
1.Don't test implementation details (avoid namespace detail::)
2. Avoid square shapes, or at least favor rectangular cases.
3. Prime number or relatives primes can help catch more edge cases (avoid accidentally getting the right answer from a bad example
(using primes or relative primes can lead to big numbers, so you may want a simpler test case, too)
| EXPECT_EQ(result, 28); | ||
| } | ||
|
|
||
| TEST(ComputeElementSpaceSize, ColumnMajor) |
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 square gives the same answer for column major or row major, so it's not really testing column major. For packed arrays, ordering doesn't change the packed size.
| EXPECT_EQ(result, 16); | ||
| } | ||
|
|
||
| TEST(ComputeElementSpaceSizeRuntime, WithPadding) |
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 two kinds of padding:
-
Padding where there is just space for extra elements. That is, you could add more elements without changing strides and get back to a packed array. These paddings often come up taking a stride slice or region out of a packed tensor. (example: shape 2,2,2 strides 1, 3, 9, which is like part of a 3,3,3 tensor)
-
Padding where there are funny offsets that don't correspond to any packed array. Those are probably worth testing just to verify there aren't any constraints on the implementation. (A simple example: shape 2,2,2 strides 1,3,7, so that elements are at offsets 0, 1, 3, 4, 7, 8, 10, 11)
- Remove tests of implementation details (detail::compute_element_space_size) - Use public API (make_naive_tensor_descriptor) for all tests - Avoid square/cube shapes that could hide row/column major bugs - Use prime numbers for padding tests to catch index calculation errors - Add two padding test cases: arbitrary offsets and stride slice
- Add tests for make_naive_tensor_descriptor_packed (1D, 2D, 3D) - Add tests for make_naive_tensor_descriptor_aligned (2D, 3D) - Add 1D tensor tests with explicit strides - Ensure all shapes use distinct, coprime dimensions
Summary
Add Google Tests for the optimized template utilities from the PR stack to ensure correctness and prevent regressions.
Tests Added
unit_sequence.cppsequence_genwith custom functorsunit_sequence_helper.cppgenerate_identity_sequences,find_in_tuple_of_sequences,sequence_find_valueunit_container_helper.cppcontainer_concat,make_uniform_tupleunit_tensor_descriptor_helper.cppcompute_element_space_sizeTotal: 43 new tests
Test Coverage by PR
sequence_genwith__make_integer_seqgenerate_identity_sequenceshelpercontainer_concat,make_uniform_tuplecompute_element_space_sizefind_in_tuple_of_sequences,sequence_find_valuePR Stack
__make_integer_seqTracking issue: #3575