31 nondeterministic rng seed in tests makes troubleshooting impossible should use c rng everywhere by SpaceyLake · Pull Request #33 · TAPPorg/reference-implementation

SpaceyLake · 2026-01-22T14:06:22Z

The main thing I did to solve the issue was to rewrite how extents are assigned. While doing that, I found more ways of improving the code, such as templating functions. I also rewrote the code for deciding the index distribution and index assignment. The code now also completely uses the new way of randomizing things. I am unsure about the Valgrind failure. I don't understand where those two possibly lost blocks originate.

change constexpr flag use_cpp_rng to tru to enable ... doing so reveals more issues in test.cc

… extent generation + minor improvements

evaleev · 2026-01-22T19:38:56Z

Looks good, thanks!

I am unsure about the Valgrind failure. I don't understand where those two possibly lost blocks originate.

Seems to be some internal scratch allocated in TBLIS? @devinamatthews https://github.com/TAPPorg/reference-implementation/actions/runs/21250446136/job/61150346309?pr=33#step:12:66

Copilot

Pull request overview

This pull request addresses issue #31 by replacing the nondeterministic RNG system with C++'s <random> library to enable reproducible test results via seeding. The changes include significant refactoring to use templates for reducing code duplication, rewriting the index distribution and extent assignment logic, and renaming variables for clarity (e.g., "unique_idx" → "isolated_idx", "idx" → "nmode" in test names).

Changes:

Implemented seeded RNG using std::mt19937 with command-line configurable seed
Templated test helper functions (run_tblis_mult, compare_tensors, generate_pseudorandom_contraction, etc.) to support multiple data types
Rewrote index and extent assignment logic with new helper functions (generate_unique_indices, assign_indices, assign_extents, generate_index_extent_map)

Reviewed changes

Copilot reviewed 1 out of 2 changed files in this pull request and generated 17 comments.

File	Description
test/test.h	Updated function declarations to use templates; renamed functions for clarity (unique_idx → isolated_idx, same_idx → same_nmode); added new helper function declarations
test/test.cpp	Implemented seeded RNG engine; templated helper functions; rewrote random contraction generation logic; updated all test functions to use new templated interfaces

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-22T19:42:57Z

test/test.cpp

    }
+    std::shuffle(unique_indices, unique_indices + total_unique_indices, std::default_random_engine()); // Shuffle the unique indices


The shuffle operations use std::default_random_engine() without a seed, which creates a new unseeded engine each time. This defeats the purpose of using a seeded RNG for reproducibility. These should use rand_engine() instead to ensure deterministic behavior. This issue occurs on lines 731, 770, 786, and 787.

Suggested change

std::shuffle(unique_indices, unique_indices + total_unique_indices, std::default_random_engine()); // Shuffle the unique indices

std::shuffle(unique_indices, unique_indices + total_unique_indices, rand_engine()); // Shuffle the unique indices using the seeded engine

Copilot · 2026-01-22T19:42:57Z

test/test.cpp

-    {
-        idx_B[i + contractions] = 'a' + nmode_A - repeated_idx_A + i;
-    }
+    std::shuffle(idx_D, idx_D + (free_indices_A + hadamard_indices + free_indices_B), std::default_random_engine()); // Shuffle indices for D


The shuffle operations use std::default_random_engine() without a seed, which creates a new unseeded engine each time. This defeats the purpose of using a seeded RNG for reproducibility. These should use rand_engine() instead to ensure deterministic behavior.

Copilot · 2026-01-22T19:42:57Z

test/test.cpp

+                    int max_contracted_indices = (((nmode_B - hadamard_indices) + (nmode_A - hadamard_indices) - (nmode_D - hadamard_indices))%2)/2;
+                }
+                else
+                {
+                    int max_contracted_indices = std::min(nmode_A, nmode_B) - hadamard_indices;


Variable shadowing issue: int max_contracted_indices is declared inside an if block at line 495, then referenced outside that block at line 499. However, there's another declaration of int max_contracted_indices at line 492 that is not used. The variable at line 495 should be an assignment, not a declaration. The current code will not compile or will have undefined behavior.

Suggested change

int max_contracted_indices = (((nmode_B - hadamard_indices) + (nmode_A - hadamard_indices) - (nmode_D - hadamard_indices))%2)/2;

}

else

{

int max_contracted_indices = std::min(nmode_A, nmode_B) - hadamard_indices;

max_contracted_indices = (((nmode_B - hadamard_indices) + (nmode_A - hadamard_indices) - (nmode_D - hadamard_indices))%2)/2;

}

else

{

max_contracted_indices = std::min(nmode_A, nmode_B) - hadamard_indices;

Copilot · 2026-01-22T19:42:58Z

test/test.cpp

-    }
-
-    if (nmode_B > 0)
+    // TODO: When repeated indices are enabled the tensors need at least one other index. This is not yet ensured.


The TODO comment indicates that when repeated indices are enabled, tensors need at least one other index, but this constraint is not yet enforced. This could lead to invalid tensor configurations. Consider implementing this validation or removing the feature flag until the constraint can be properly enforced.

Copilot · 2026-01-22T19:42:58Z

test/test.h

+bool test_subtensor_same_nmode();
+bool test_subtensor_lower_nmode();


Inconsistent naming: The function parameter names use "nmode" (e.g., subtensor_on_nmode at line 57), but the actual variable being modified/used is often the number of modes. However, the test function names were changed from "test_subtensor_same_idx" to "test_subtensor_same_nmode" (line 118), but these functions test whether subtensors have the same number of modes as their parent tensors, not whether indices are the same. The naming change improves clarity, but "nmode" typically refers to "number of modes", so "same_nmode" might be better expressed as "matching_nmode" or "unchanged_nmode" for better readability.

Copilot · 2026-01-22T19:43:00Z

test/test.h

 bool test_contraction_double_precision();
 bool test_contraction_complex();
 bool test_contraction_complex_double_precision();
 bool test_zero_stride();


Function name changed from "test_unique_idx" to "test_isolated_idx" (line 130), but the PR description mentions "rewriting code for deciding the index distribution and index assignment" without explaining the semantic difference between "unique" and "isolated" indices. The function implementation suggests these are indices that appear in only one tensor. Consider adding a comment explaining what "isolated indices" means in this context.

Suggested change

bool test_zero_stride();

bool test_zero_stride();

// "Isolated indices" are indices that appear in exactly one tensor (i.e., they are not shared or contracted with any other tensor).

Copilot · 2026-01-22T19:43:00Z

test/test.cpp

+                                                       isolated_indices_A, isolated_indices_B,
+                                                       repeated_indices_A, repeated_indices_B);
+
+    std::unordered_map<int, int64_t> index_extent_map = generate_index_extent_map(min_extent, 4, total_unique_indices, unique_indices);


In generate_pseudorandom_contraction, the equal_extents_only parameter is passed in but never used within the function. It's passed to generate_index_extent_map at line 301, but that function doesn't accept such a parameter. The equal_extents_only logic needs to be implemented, likely in the generate_index_extent_map function or before calling it.

Suggested change

std::unordered_map<int, int64_t> index_extent_map = generate_index_extent_map(min_extent, 4, total_unique_indices, unique_indices);

std::unordered_map<int, int64_t> index_extent_map;

if (equal_extents_only) {

// Assign the same extent to all unique indices

int64_t common_extent = rand(min_extent, 4);

for (int64_t i = 0; i < total_unique_indices; ++i) {

index_extent_map[unique_indices[i]] = common_extent;

}

} else {

index_extent_map = generate_index_extent_map(min_extent, 4, total_unique_indices, unique_indices);

}

Copilot · 2026-01-22T19:43:01Z

test/test.cpp

+        static_assert(std::is_same_v<T, void>,
+                      "rand<T>: unsupported type");
+    }


The static_assert at line 1060 uses std::is_same_v<T, void> as a condition that will always be false (since T is the actual type passed to the template). This appears to be an intentional way to generate a compile-time error for unsupported types, but the error message could be misleading. A better approach would be to use a false-valued type trait or simply false with a clearer error message like "Unsupported type for rand function".

Copilot · 2026-01-22T19:43:01Z

test/test.cpp

+                    int max_contracted_indices = (((nmode_B - hadamard_indices) + (nmode_A - hadamard_indices) - (nmode_D - hadamard_indices))%2)/2;
+                }
+                else
+                {
+                    int max_contracted_indices = std::min(nmode_A, nmode_B) - hadamard_indices;


The calculation for max_contracted_indices appears incorrect. The expression (((nmode_B - hadamard_indices) + (nmode_A - hadamard_indices) - (nmode_D - hadamard_indices))%2)/2 divides the modulo result by 2. Since modulo 2 can only be 0 or 1, dividing by 2 will always yield 0 (integer division). This doesn't seem like the intended logic for calculating the maximum number of contracted indices. The formula should likely be ((nmode_B - hadamard_indices) + (nmode_A - hadamard_indices) - (nmode_D - hadamard_indices))/2 without the modulo operation.

Suggested change

int max_contracted_indices = (((nmode_B - hadamard_indices) + (nmode_A - hadamard_indices) - (nmode_D - hadamard_indices))%2)/2;

}

else

{

int max_contracted_indices = std::min(nmode_A, nmode_B) - hadamard_indices;

max_contracted_indices = ((nmode_B - hadamard_indices)

+ (nmode_A - hadamard_indices)

- (nmode_D - hadamard_indices)) / 2;

}

else

{

max_contracted_indices = std::min(nmode_A, nmode_B) - hadamard_indices;

Copilot · 2026-01-22T19:43:01Z

test/test.cpp

+T rand()
 {
-    return std::complex<float>(rand_s(), rand_s());
+    return rand<T>(-RAND_MAX, RAND_MAX);
 }


In the rand template function, when the input types are integers or floating-point values, the function uses RAND_MAX for the default range. However, RAND_MAX is a macro from the C standard library (typically 32767 or 2147483647) and may not be appropriate for all template types T. For example, if T is int64_t and RAND_MAX is 32767, the range would be limited. Consider using std::numeric_limits<T>::max() instead for better type safety.

evaleev · 2026-01-22T19:54:48Z

@SpaceyLake some of the copilot comments look useful ...

evaleev · 2026-01-23T16:04:27Z

Looks good, thanks!

I am unsure about the Valgrind failure. I don't understand where those two possibly lost blocks originate.

Seems to be some internal scratch allocated in TBLIS? @devinamatthews https://github.com/TAPPorg/reference-implementation/actions/runs/21250446136/job/61150346309?pr=33#step:12:66

@SpaceyLake #34 implements a workaround this issue, rebase on master so that valgrind test can pass also ...

devinamatthews · 2026-01-23T16:32:33Z

You would need to call bli_finalize to clean those up.

change constexpr flag use_cpp_rng to tru to enable ... doing so reveals more issues in test.cc

… extent generation + minor improvements

…ting-impossible-should-use-c-rng-everywhere' of github.com:TAPPorg/reference-implementation into 31-nondeterministic-rng-seed-in-tests-makes-troubleshooting-impossible-should-use-c-rng-everywhere

change constexpr flag use_cpp_rng to tru to enable ... doing so reveals more issues in test.cc

… extent generation + minor improvements

…ting-impossible-should-use-c-rng-everywhere' of github.com:TAPPorg/reference-implementation into 31-nondeterministic-rng-seed-in-tests-makes-troubleshooting-impossible-should-use-c-rng-everywhere

evaleev · 2026-01-23T18:52:58Z

You would need to call bli_finalize to clean those up.

unfortunately can't include <blis.h> alongside <tblis.h> to be able to call it safely:

devinamatthews · 2026-01-23T19:04:49Z

That's a good point please raise an issue on TBLIS. A temporary workaround is to include our own prototype for bli_finalize. From: Eduard Valeyev ***@***.***> Date: Friday, January 23, 2026 at 12:53 PM To: TAPPorg/reference-implementation ***@***.***> Cc: Matthews, Devin ***@***.***>, Mention ***@***.***> Subject: Re: [TAPPorg/reference-implementation] 31 nondeterministic rng seed in tests makes troubleshooting impossible should use c rng everywhere (PR #33) [https://avatars.githubusercontent.com/u/2192487?s=20&v=4]evaleev left a comment (TAPPorg/reference-implementation#33)<#33 (comment)> You would need to call bli_finalize to clean those up. unfortunately can't include <blis.h> alongside <tblis.h> to be able to call it safely: image.png (view on web)<https://github.com/user-attachments/assets/0c1f2fa7-b74f-4340-8e08-93d6ff032bc8> — Reply to this email directly, view it on GitHub<#33 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABIAZILBOI5FJVYEHZMLNAT4IJUZ7AVCNFSM6AAAAACSRMTGF6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTOOJRG44TGMZZG4>. You are receiving this because you were mentioned.

The packm_bsmtc functions in TBLIS/BLIS trigger "Conditional jump depends on uninitialised value" errors that appear to be false positives in architecture-specific packing code. The suppression uses wildcards to match any architecture variant (zen3, haswell, etc.). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

…ting-impossible-should-use-c-rng-everywhere' of github.com:TAPPorg/reference-implementation into 31-nondeterministic-rng-seed-in-tests-makes-troubleshooting-impossible-should-use-c-rng-everywhere

evaleev · 2026-01-23T19:22:03Z

@SpaceyLake finally, CI passes. Let's merge (unless copilot has something useful to say) and lock the master for CI-breaking merges.

Copilot

Pull request overview

Copilot reviewed 2 out of 4 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-23T19:26:35Z

test/test.cpp

+unsigned int current_rand_seed = 0;
+auto& rand_engine() {
+    static std::mt19937 engine(current_rand_seed);
+    return engine;


The PR title mentions using a C RNG everywhere, but the updated tests now use std::mt19937/<random> instead. If the intent is truly to use the C RNG, this is a mismatch; otherwise consider updating the PR title/description to reflect the switch to the C++ random engine.

Copilot · 2026-01-23T19:26:36Z

test/test.cpp

+    if constexpr (std::is_same_v<T, float>)
+    {
+        tblis_init_tensor_scaled_s(&tblis_A, alpha, nmode_A, tblis_len_A, A, tblis_stride_A);
+        tblis_init_tensor_s(&tblis_B, nmode_B, tblis_len_B, B, tblis_stride_B);
+        tblis_init_tensor_scaled_s(&tblis_C, beta, nmode_C, tblis_len_C, C, tblis_stride_C);
+        tblis_init_tensor_scaled_s(&tblis_D, 0, nmode_D, tblis_len_D, D, tblis_stride_D);
+    }
+    else if constexpr (std::is_same_v<T, double>)
+    {
+        tblis_init_tensor_scaled_d(&tblis_A, alpha, nmode_A, tblis_len_A, A, tblis_stride_A);
+        tblis_init_tensor_d(&tblis_B, nmode_B, tblis_len_B, B, tblis_stride_B);
+        tblis_init_tensor_scaled_d(&tblis_C, beta, nmode_C, tblis_len_C, C, tblis_stride_C);
+        tblis_init_tensor_scaled_d(&tblis_D, 0, nmode_D, tblis_len_D, D, tblis_stride_D);
+    }
+    else if constexpr (is_complex_v<T>) 
+    {
+        using value_type = typename T::value_type;
+        if constexpr (std::is_same_v<value_type, float>)
+        {
+            tblis_init_tensor_scaled_c(&tblis_A, alpha, nmode_A, tblis_len_A, A, tblis_stride_A);
+            tblis_init_tensor_c(&tblis_B, nmode_B, tblis_len_B, B, tblis_stride_B);
+            tblis_init_tensor_scaled_c(&tblis_C, beta, nmode_C, tblis_len_C, C, tblis_stride_C);
+            tblis_init_tensor_scaled_c(&tblis_D, 0, nmode_D, tblis_len_D, D, tblis_stride_D);


run_tblis_mult takes op_A/op_B/op_C/op_D but never applies them to the created tblis_tensor objects (e.g., setting .conj). This means the reference TBLIS computation ignores conjugation for complex tensors, so test_contraction_complex* can produce incorrect comparisons. Apply the op flags to the appropriate tblis_* tensors (or drop these parameters if they are intentionally unsupported).

test/test.cpp

test/test.h

…ting-impossible-should-use-c-rng-everywhere' of github.com:TAPPorg/reference-implementation into 31-nondeterministic-rng-seed-in-tests-makes-troubleshooting-impossible-should-use-c-rng-everywhere

SpaceyLake · 2026-01-26T13:32:35Z

Did some fixes on copilots comments. The one about the op parameters, I feel, can be fixed later. No test tests the elemental operations anyway, right now. If I were to change the call to TBLIS, I would like to do a proper mapping to TAPP anyway. I believe we can merge this to main now.

evaleev and others added 2 commits January 15, 2026 10:22

test.cc: can use C++ RNG throughout (disabled by default)

4a0df7a

change constexpr flag use_cpp_rng to tru to enable ... doing so reveals more issues in test.cc

Major test revision: randomization, template functions, new index and…

e5ef0b6

… extent generation + minor improvements

SpaceyLake requested a review from evaleev January 22, 2026 14:06

SpaceyLake linked an issue Jan 22, 2026 that may be closed by this pull request

nondeterministic RNG seed in tests makes troubleshooting impossible, should use C++ RNG everywhere #31

Closed

evaleev approved these changes Jan 22, 2026

View reviewed changes

evaleev requested a review from Copilot January 22, 2026 19:36

Copilot started reviewing on behalf of evaleev January 22, 2026 19:37 View session

Copilot AI reviewed Jan 22, 2026

View reviewed changes

Fixes for review

48ebbdf

evaleev and others added 14 commits January 23, 2026 18:38

test.cc: can use C++ RNG throughout (disabled by default)

e2e8b12

change constexpr flag use_cpp_rng to tru to enable ... doing so reveals more issues in test.cc

Major test revision: randomization, template functions, new index and…

3829f9b

… extent generation + minor improvements

Fixes for review

c534d3a

Corrected function declaration in include file

6cf01ba

Ignores the build folder

922747e

Merge branch '31-nondeterministic-rng-seed-in-tests-makes-troubleshoo…

2cd62ec

…ting-impossible-should-use-c-rng-everywhere' of github.com:TAPPorg/reference-implementation into 31-nondeterministic-rng-seed-in-tests-makes-troubleshooting-impossible-should-use-c-rng-everywhere

Removed type check

b85a624

test.cc: can use C++ RNG throughout (disabled by default)

0489fd2

change constexpr flag use_cpp_rng to tru to enable ... doing so reveals more issues in test.cc

Major test revision: randomization, template functions, new index and…

1a9a39a

… extent generation + minor improvements

Fixes for review

a91decd

Corrected function declaration in include file

d07a107

Ignores the build folder

6c94692

Removed type check

42ea6bc

Merge branch '31-nondeterministic-rng-seed-in-tests-makes-troubleshoo…

0b85e08

…ting-impossible-should-use-c-rng-everywhere' of github.com:TAPPorg/reference-implementation into 31-nondeterministic-rng-seed-in-tests-makes-troubleshooting-impossible-should-use-c-rng-everywhere

amend 675391e, no need to suppress blis leaks, call bli_finalize instead

5c4ec8f

evaleev and others added 2 commits January 23, 2026 14:15

Merge branch '31-nondeterministic-rng-seed-in-tests-makes-troubleshoo…

22d72c9

…ting-impossible-should-use-c-rng-everywhere' of github.com:TAPPorg/reference-implementation into 31-nondeterministic-rng-seed-in-tests-makes-troubleshooting-impossible-should-use-c-rng-everywhere

evaleev requested a review from Copilot January 23, 2026 19:21

Copilot started reviewing on behalf of evaleev January 23, 2026 19:21 View session

Copilot AI reviewed Jan 23, 2026

View reviewed changes

SpaceyLake added 4 commits January 26, 2026 14:16

Removed duplicate of including random

74b8dfd

Corrected input types from pointers to values

0b0a310

Corrected rand function to work for complex types

dbc9b6d

Merge branch '31-nondeterministic-rng-seed-in-tests-makes-troubleshoo…

5fbc877

…ting-impossible-should-use-c-rng-everywhere' of github.com:TAPPorg/reference-implementation into 31-nondeterministic-rng-seed-in-tests-makes-troubleshooting-impossible-should-use-c-rng-everywhere

evaleev merged commit ce1a4f7 into main Jan 26, 2026
12 checks passed

evaleev deleted the 31-nondeterministic-rng-seed-in-tests-makes-troubleshooting-impossible-should-use-c-rng-everywhere branch January 26, 2026 14:38

		}
		std::shuffle(unique_indices, unique_indices + total_unique_indices, std::default_random_engine()); // Shuffle the unique indices

		bool test_subtensor_same_nmode();
		bool test_subtensor_lower_nmode();

	bool test_zero_stride();
	bool test_zero_stride();
	// "Isolated indices" are indices that appear in exactly one tensor (i.e., they are not shared or contracted with any other tensor).

-    std::unordered_map<int, int64_t> index_extent_map = generate_index_extent_map(min_extent, 4, total_unique_indices, unique_indices);
+    std::unordered_map<int, int64_t> index_extent_map;
+    if (equal_extents_only) {
+        // Assign the same extent to all unique indices
+        int64_t common_extent = rand(min_extent, 4);
+        for (int64_t i = 0; i < total_unique_indices; ++i) {
+            index_extent_map[unique_indices[i]] = common_extent;
+        }
+    } else {
+        index_extent_map = generate_index_extent_map(min_extent, 4, total_unique_indices, unique_indices);
+    }

Conversation

SpaceyLake commented Jan 22, 2026

Uh oh!

evaleev commented Jan 22, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

evaleev commented Jan 22, 2026

Uh oh!

evaleev commented Jan 23, 2026

Uh oh!

devinamatthews commented Jan 23, 2026

Uh oh!

evaleev commented Jan 23, 2026

Uh oh!

devinamatthews commented Jan 23, 2026 via email

Uh oh!

evaleev commented Jan 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SpaceyLake commented Jan 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants