r.param.scale: add reference-value pytest#7534
Conversation
Runs the module on a small generated deterministic DEM for every method, window size and the -c flag, and compares univariate statistics against reference values captured from the current main. Verifies that changes such as the OpenMP parallelization do not alter the output.
github-actions
Bot
added
raster
|
How long does the test run? Practically, we can't have long running tests (e.g. more than 10 seconds) in the CI. |
Reduce the test to a 50x50 region and 7 unconstrained methods (elev, slope, aspect, profc, planc, minic, feature), with references regenerated from the main binary at OMP_NUM_THREADS=1. Drop the constrained (-c) combinations, whose ill-conditioned fit diverged across platforms beyond a usable tolerance. Also remove the dead isinstance guard and the unnecessary grassdata folder per review.
It runs in about 4 seconds locally now. In this update I shrank the region to 50x50, dropped the -c cases because the constrained fit isn't stable across platforms, and reduced the number of methods from 10 to just 7. That ends up being 14 small runs, so the whole test stays well under 10 seconds. If you could approve the workflows, I can also check the exact CI runtime in the logs. Thanks. |
petrasovaa
left a comment
petrasovaa
left a comment
There was a problem hiding this comment.
Let's merge this, but it would be good to understand the -c flag.
|
There's something weird and transient in that new test, and it's not something small like absolute error to relax. I've restarted multiple pytest jobs in PRs or main since that was merged, with failures making no sense. https://github.com/OSGeo/grass/actions/runs/27877100569/job/82520365322 It's off by a large margin. The failure wasn't always that specific one. https://github.com/OSGeo/grass/actions/runs/27877100569/job/82498639669 |
After a lot of stress testing I traced the transient problems to the race in libgmath (G_ludcmp) which is #7539, not the test or r.param.scale. The pivot search runs in parallel but updates its shared best row variables with no locking, so the pivot it picks depends on thread timing. Most of the time it's harmless, but once in a while it takes a row that’s almost zero and the whole solution blows up, which is where the -72702 comes from. At OMP_NUM_THREADS=1 it's identical every run, it only drifts above one thread. I wrote up the details and how to reproduce it in #7539. But I think planc-9 is a smaller problem. It's only off by about 2.6e-9 with the bound at 2.1e-9, so that's just rounding noise, not the blowup. That run seems to be on the old tolerance. You can see the 2.1e-9 bound in the output. #7584 replaced it with a 5e-8 floor for the near-zero curvature methods, so it's under the floor now and shouldn't come back. For now I can put up a small PR to skip the test until the library fix works, the same way r.mapcalc's nprocs test is already skipped ("Waiting for a fix to work on all platforms"), with #7539 tracking it. |
|
Thanks for taking a look! So that new test uncovered something ;) |
3 participants
This PR adds a pytest for r.param.scale to check that changes like the OpenMP parallelization in #7440 don't change the output.
The test generates a small deterministic DEM with r.mapcalc (a polynomial in row() and col(), so it is identical on every platform) on a 50x50 region. It runs r.param.scale across 7 methods (elev, slope, aspect, profc, planc, minic, feature) at window sizes 3 and 9. This is 14 combinations in total. For each combination it checks the r.univar stats against reference values. The cell and null counts have to match exactly, and the value stats need to be within a small tolerance.
The reference values were generated with a build of current main, so they represent the output as it is right now. The same test passes on main and on the parallel branch from #7440, which shows the parallelization isn't changing anything.
Follows the pytest conventions of raster/r.slope.aspect/tests.