Repository navigation
tests: Migrate r.mapcalc rand seed tests to pytest - #7633
Merged
Merged
Conversation
This migrates the r.mapcalc gunittest tests to pytest (NC SPM is not used) and puts all random number generator function tests into one file. At this point, both single threaded and parallel parameters give the same numbers because r.mapcalc uses threads=1 even when user specified nprocs!=1 to avoid races in the underlying random number generator. In these cases, a verbose message is produced and a new, separate test tests for that. I used Claude Fable to generate the new code.
github-actions
Bot
added
raster
echoix
approved these changes
Jul 4, 2026
ninsbl
pushed a commit
to ninsbl/grass
that referenced
this pull request
Jul 7, 2026
wenzeslaus
added a commit
to wenzeslaus/grass
that referenced
this pull request
Aug 18, 2026
Replace the pthread mutex approach with a lock-free compare-and-swap on the whole 48-bit generator state kept in a single C11 atomic integer. One successful compare-and-swap is exactly one step of the standard drand48 linear congruential generator, so the sequence of generated values for a given seed is identical to a single-threaded run and to the previous limb-based implementation. Compared to the mutex: the feature is gated by the compiler (__STDC_NO_ATOMICS__), not by --with-pthread, so it is active in every supported build, including the OpenMP-without-pthread configurations (macOS CI, Docker images) where the race reported in OSGeo#5742 actually occurs; no build system changes or extra link dependencies are needed (the Makefile and CMake changes are dropped, which also avoids defining HAVE_PTHREAD from the Win32-threads CMake package on MSVC); and there is no lock contention (in a microbenchmark the mutex made threaded runs slower than single-threaded at every thread count, while the atomic version is about twice as fast as the mutex when several threads generate numbers at the same time). A single shared generator still cannot scale with the number of threads; per-thread state remains future work. Where C11 atomics are unavailable, the code falls back to the previous plain implementation, which is not thread-safe (unchanged behavior). Output compatibility is guarded by the PRNG regression tests merged in OSGeo#7633 and OSGeo#7634 (lib/gis/tests/lib_gis_lrand48_test.py and raster/r.mapcalc/tests/r_mapcalc_rand_seed_test.py) and was additionally verified against the previous implementation for multiple seeds. The 64-bit reformulation of the generator and its verification were AI-assisted (Claude).
wenzeslaus
added a commit
to wenzeslaus/grass
that referenced
this pull request
Aug 18, 2026
Replace the pthread mutex approach with a lock-free compare-and-swap on the whole 48-bit generator state kept in a single C11 atomic integer. One successful compare-and-swap is exactly one step of the standard drand48 linear congruential generator, so the sequence of generated values for a given seed is identical to a single-threaded run and to the previous limb-based implementation. Compared to the mutex: the feature is gated by the compiler (__STDC_NO_ATOMICS__), not by --with-pthread, so it is active in every supported build, including the OpenMP-without-pthread configurations (macOS CI, Docker images) where the race reported in OSGeo#5742 actually occurs; no build system changes or extra link dependencies are needed (the Makefile and CMake changes are dropped, which also avoids defining HAVE_PTHREAD from the Win32-threads CMake package on MSVC); and there is no lock contention (in a microbenchmark the mutex made threaded runs slower than single-threaded at every thread count, while the atomic version is about twice as fast as the mutex when several threads generate numbers at the same time). A single shared generator still cannot scale with the number of threads; per-thread state remains future work. Where C11 atomics are unavailable, the code falls back to the previous plain implementation, which is not thread-safe (unchanged behavior). Output compatibility is guarded by the PRNG regression tests merged in OSGeo#7633 and OSGeo#7634 (lib/gis/tests/lib_gis_lrand48_test.py and raster/r.mapcalc/tests/r_mapcalc_rand_seed_test.py) and was additionally verified against the previous implementation for multiple seeds. The 64-bit reformulation of the generator and its verification were AI-assisted (Claude).
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.
This migrates the r.mapcalc gunittest tests to pytest (NC SPM is not used) and puts all random number generator function tests into one file. At this point, both single threaded and parallel parameters give the same numbers because r.mapcalc uses threads=1 even when user specified nprocs!=1 to avoid races in the underlying random number generator. In these cases, a verbose message is produced and a new, separate test tests for that.
The new code drops various todos (more tests - mostly obvious) and comments (tested using NumPy, not r.mapcalc, so not needed anymore).
Whatever new behavior we may decide to deal with multi-threading, these tests clearly show the current behavior.
I used Claude (Fable) to generate the new code.