Repository navigation
Conversation
…#2807) Per-ray seeding used srandom(_hash((pidx+1)*(seed+1))). Because the key is a product, ray p at seed 3 equals ray 2p+1 at seed 1, MPI ranks (seed+rank) share up to ~45% of their rays, and with 32-bit unsigned long (MSVC) the key wraps at 2^32 (seed 2147483647 gives two rays). Use srandom(_hash(_hash(seed) + cloop*gpu_innerloop + pidx)) in both raytrace_all and raytrace_all_funnel. _hash is a 64-bit bijection, so distinct (seed, global ray index) pairs give distinct KISS seeds. The per-batch "seed += gpu_innerloop" is dropped; the global ray index replaces it (otherwise seed S batch 1 == seed S+gpu_innerloop batch 0). NOTE: this changes numerical results of every KISS run (default RNG) for a given seed. Mersenne Twister builds are unaffected. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
_uid was pidx (+ rank*ncount), so it repeated in every GPU batch, and the rank stride ncount could be smaller than the loops*gpu_innerloop rays a rank actually traces when batching rounds ncount up. Use the global ray index cloop*gpu_innerloop + pidx and a rank stride of loops*gpu_innerloop. On CPU (loops=1, gpu_innerloop=ncount) uids are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The particle struct had randstate_t randstate[7] hard-coded, and sort_absorb_last() (FUNNEL + SPLIT) copied randstate[0..6] by hand. Use RANDSTATE_LEN for both, so an RNG with a different state length neither wastes space nor indexes out of bounds. No change in results: KISS keeps RANDSTATE_LEN 7 (Test_RNG_rand01 md5 still 6c203d69205b0656c1391555e6068c93); MT ignores the per-particle state. Unittest_SPLIT, legacy and FUNNEL, ALG 1/2: ASan+UBSan clean. ADR updated. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
McStas test-suite: https://github.com/mccode-dev/McCode/actions/runs/39104410513 McXtrace test-suite: https://github.com/mccode-dev/McCode/actions/runs/39104430311 |
Both references came from runs with the old product-key seeding, and both tests are low-statistics. Checked against a referee that does not use per-ray seeding at all (Mersenne Twister, RNG_ALG=1), many seeds: Test_SAXS SAMPLE=7 (SAXSPDB, 1e4 rays, serial): MT 5.37e-14 +- 0.16 (12 seeds), new 5.66e-14 +- 0.20 (16), old 6.41e-14 +- 0.28 (16): the old seeding was ~3 sigma high. Reference 7.07865e-14 -> 5.54e-14 (new+MT mean). CI gives 5.44e-14. templateSANS_Mantid (1e6 rays, 2 MPI ranks as in CI): MT 9.51e7 +- 0.39 (16 seeds), new 9.77e7 +- 0.32, old 10.42e7 +- 0.27. One run scattered +-15% (~1150 events), so CI (seed 1000) landed at 7.47e7. SPLIT 10 -> SPLIT 100 on the sample cuts that to ~5% (0.8 s -> 2.2 s; the expected intensity is unchanged), reference 1.05e8 -> 9.6e7; seed 1000 now gives 9.94e7 (np 2), 9.55e7 (np 4), 9.41e7 (serial). mctest ignores -n in %Example for non-scan tests, so raising ncount there was not an option. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
All values from the CI run on this branch (identical on macOS and Ubuntu; 0 discrepancies on main) were checked against a referee that does not use per-ray seeding (Mersenne Twister, RNG_ALG=1), 8 seeds, 1e6 rays. The new seeding agrees with MT in every case. FZJ_BenchmarkSfin2: MT 8136 +- 280, new 8832 +- 323, old 9464 +- 427. A run scatters 10-13%; the quoted error (1%) is ~10x too small because of SPLIT 2000 correlations, hence the misleading "17.9 sigma". Reference 7400 -> 8.5e3 (CI 10014 = 118%). Test_SANS SAMPLE=0: MT 3.03e-9 +- 0.13, new 2.78e-9 +- 0.18, old 2.50e-9 +- 0.15. Reference 2.529e-9 -> 2.9e-9 (CI 3.27e-9 = 113%). Test_Magnon_bcc_TAS: ~100 events, 16-22% scatter. SPLIT 20 on the sample: ~1900 events, 5-7%; MT 5.03e5, new 4.90e5. Reference 4.6e5 -> 4.97e5 (CI 5.08e5 = 102%). The %Scan still passes 21/21 with its existing references (~33 s with 2 ranks). Test_TasReso: ~13 events, ~45% scatter. SPLIT 50 on sample_arm1 (before the three WHEN samples): ~580 events, 6-8%; MT 936, new 982. Reference 887.137 -> 960 (CI 995 = 104%). SPLIT does not change expected intensities; runs take ~2 s. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The 5x5 mesh h,l = 0.6..1.4 put points on the edges of the dispersion ring (outer edge ~0.65/1.35, an inner hole near h=1.0, l=0.9-1.0). Points there have reference 0 but occasionally catch a stray event: in CI point 22 (h=1.4, l=1.0) got one (7e-15 +- 5e-15), which mctest can never accept against a 0 target. 0 of 80 independent runs gave any event there, and whether it happens even depends on compiler flags. New mesh h=0.8..1.2, l=1.1..1.3 (5x5, 2e5 rays per point, unchanged) lies inside the band: every point has 280-2000 events and scatters 1-5% per run. References are the mean of 8 seeds with the new seeding and 8 with Mersenne Twister (RNG_ALG=1); the two agree at every point (worst 1.8 sigma). With mcrun's own build flags and 2 MPI ranks, seed 1000 (as in CI) and seeds 1 and 2 all give 25/25 points within 94-106%. References are listed in mcrun's point order (h fastest). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
@g5t, @mads-bertelsen, @farhi FYI patch incoming on KISS RNG. Will likely keep this open until after an evaluation tomorrow. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Meant to eventually fix #2807.
Warning
Numerical behaviour change: every run using the default KISS RNG will now give different results for a given
--seedthan before. Statistics are unchanged (and better for MPI and small seeds), but outputs are not bit-identical to earlier releases. Builds with the Mersenne Twister RNG (RNG_ALG=1) are not affected.Problem
In the generated
raytrace_allandraytrace_all_funnel, each ray was seeded withsrandom(_hash((pidx+1)*(seed+1))). Because the key is a product:pat seed 3 is identical to ray2p+1at seed 1. In general, runs at different seeds share rays.mcseed + rank, so the ranks of one run overlap: with-np 8 --seed 1only ~55% of rays are distinct.unsigned longis 32 bits, so the key wraps at 2^32. Seed 2147483647 alternates between just two rays.Change 1: per-ray seed (code generator only)
_hash(existing Mueller hash,randstate_t=uint64_tfor KISS) is a 64-bit bijection, so distinct(seed, global ray index)pairs give distinct KISS seeds. The sum is computed in 64 bits on every platform, MSVC included.seed += gpu_innerloopis removed. The global ray indexcloop*gpu_innerloop + pidxreplaces it. Without that, seed S batch 1 would equal seed S+gpu_innerloop batch 0._hashis already#pragma acc routine seq.Change 2:
_uidunique across GPU batches and MPI ranksPreviously
_uid = pidx(+rank*ncount), so uids repeated in every GPU batch. The rank stridencountcould also be smaller than theloops*gpu_innerlooprays a rank actually traces when batching roundsncountup. Now:On CPU (
loops=1,gpu_innerloop=ncount) the uids are unchanged. Checked: serial 1000 rays give uids 0..999 (normal loop and FUNNEL),mpirun -np 4with 4000 rays gives unique uids 0..3999. The batched GPU path is not exercised locally.Change 3: particle
randstatesized byRANDSTATE_LENThe particle struct had
randstate_t randstate[7]hard-coded, andsort_absorb_last()(FUNNEL + SPLIT,mccode-r.c) copiedrandstate[0..6]by hand. Both now useRANDSTATE_LEN. Results don't change: KISS keeps 7 words (Test_RNG_rand01md5 still6c203d69…) and the MT struct shrinks to 1 word. This makes room for an RNG with another state length, e.g. the TinyMT64 GPU generator being prototyped separately. Checked:Unittest_SPLIT, legacy and FUNNEL,RNG_ALG1 and 2, clean under ASan and UBSan.Test references updated
Each failing low-statistics test was checked against a referee that does not use per-ray seeding at all: Mersenne Twister (
RNG_ALG=1, one global stream), over many seeds.Test_SAXS8 (SAXSPDB, 1e4 rays)templateSANS_Mantid1 and 2 (1e6 rays, 2 MPI ranks as in CI)SPLIT 100+ reference 1.05e8 → 9.6e7FZJ_BenchmarkSfin2(SPLIT 2000: the quoted 1% error is ~10× too small)Test_SANS1Test_Magnon_bcc_TAS1 (~100 events)SPLIT 20on the sample (~1900 events) + reference 4.6e5 → 4.97e5 (CI 102%); its%Scanstill passes 21/21Test_TasReso(~13 events)SPLIT 50before the samples (~580 events) + reference 887 → 960 (CI 104%)The new seeding agrees with the referee in every case. The old seeding was about 3σ high for SAXSPDB (in a single process), and about 2σ high for Mantid with MPI, so the old references inherited a bias. A Mantid run scattered ±15%, which is why CI with seed 1000 landed at 7.47e7. With
SPLIT 100on the sample the scatter is ~5% at 2.2 s per run (was 0.8 s), and seed 1000 gives 9.94e7 (2 ranks), 9.55e7 (4) and 9.41e7 (serial). Raising ncount was not an option, becausemctestignores-nin%Examplefor non-scan tests.McStas
Test_Dispersion_relation_TAStest 6 (a%Scan): the old 5×5 mesh (h, l = 0.6–1.4) put points on the edges of the dispersion ring. Point 22 has reference 0 and in CI caught a single stray event, whichmctestcan never accept against a 0 target. The mesh now covers h = 0.8–1.2, l = 1.1–1.3, inside the band: every point gets 280–2000 events and scatters 1–5% per run. The references are the mean of 8 seeds each with new seeding and MT, which agree at every point (worst 1.8σ). Seed 1000 (as in CI) and seeds 1 and 2 give 25/25 points within 94–106%.Verification (macOS arm64, clang, CPU)
Standalone check using the runtime's verbatim
_hash/kiss_srandom/kiss_random. A ray's fingerprint is its first 3 draws:-np 8), 125k rays/rankGenerated instrument (
Progress_bar+Armwith an EXTEND that prints_uid,rand01(),rand01()):-DFUNNEL.mpirun -np 8 -s 1: old 55.6% distinct, new 100%.Not tested: MSVC, Linux gcc/clang, nvc/OpenACC GPU. CI covers the first two.
In progress: McStas and McXtrace test-suite runs are under way to measure how much results change across the instrument suites.
ADR:
docs/GRAMMAR/ADR-records/ADR_20261018_PER_RAY_SEEDING.md. mccode-antlr needs the same change ported by hand: mccode-dev/mccode-antlr#392.🤖 Generated with Claude Code