grass.mapcalc: Fix mapcalc wrapper random seeding

[y-sudharshan]

This branch adds parameter support to the mapcalc() and mapcalc_start() wrapper functions in GRASS GIS, enabling users to pass CLI flags like [-s] for automatic PRNG seeding when using the rand() function in raster calculations. The implementation modifies the main raster module to accept a [flags=""] parameter in both functions. It forwards it to the underlying command calls with updated docstrings. At the same time, a new comprehensive test suite provides 8 test methods across two test classes that verify both synchronous and asynchronous functionality with seed parameters, flag parameters, and CLI constraint validation. The fix is minimal, backward compatible (default empty string). It solves the original issue where calling the raster calculator with rand() would fail with a "not seeded" error by allowing users to now use automatic seeding via flags, seed parameters, or explicit seed values for flexible PRNG seeding options.

[petrasovaa]

I think this is a good solution, but I agree with @baharmon in the original issue that the expectation might be to autoseed by default. So perhaps adding the -s flag by default would be good. I would like to deprecate seed="auto", it does not fit the current r.mapcalc interface.

Also please check our CONTRIBUTING.md, specifically our programming guidelines for pre-commit to avoid any formatting issues.

The pytests don't run, see if you can fix that.

Thanks!

[wenzeslaus]

We eventually need to autoseed in r.mapcalc itself. That's the new approach we are taking elsewhere.

[y-sudharshan]

So, for the current wrapper implementation, should the Python wrapper automatically add the -s flag (auto-seeding) when no explicit seed is provided, or should users be required to pass flags="s" manually when using rand()?

[petrasovaa]

How about implementing this in a single PR:

• modify r.mapcalc itself to autoseed - previously you needed to provide seed= or -s if using rand() but now it would autoseed even without -s flag (provided seed was not given). We would start phasing out -s flag so that it can be eventually removed - as an example of that look at v.category -g flag (search for 'deprecated' in v.category/main.c).
• in the python mapcalc wrapper, we need to keep the seed option (for passing the seed and for backwards compatibility), so don't remove it. But seed=auto could be ignored because the new r.mapcalc autoseeding would kick in. Also we then don't need passing the flags.

I hope this makes sense now.

This change would happen on main, we would not backport it to 8.4 because of the changes in r.mapcalc, so technically to fix this in current stable branch we need a separate solution that touches only the mapcalc wrapper, but that would be a different PR.

[y-sudharshan]

The most recent push includes the following updates:
Added a flags parameter to mapcalc() and mapcalc_start() to allow passing r.mapcalc CLI flags
Handled the deprecated seed="auto" case gracefully by converting it to None and deferring seeding to r.mapcalc
Updated docstrings to clarify that r.mapcalc is expected to auto-seed when no explicit seed is provided
Updated tests to verify auto-seeding behavior as well as backwards compatibility
Maintained backwards compatibility for existing users still passing seed="auto"

Once auto-seeding is implemented directly in r.mapcalc, this will become the permanent solution and remove the need for any wrapper-level handling.

For the current PR, could you please review the latest changes and let me know if any further revisions are needed?

Thanks!

[petrasovaa]

@y-sudharshan if you please reread my comment, I was suggesting to add the autoseeding in r.mapcalc code itself: https://github.com/OSGeo/grass/blob/main/raster/r.mapcalc/main.c

[petrasovaa]

I implemented the changes in r.mapcalc in a separate PR #6742 because that turned out be more complex. So then here please include only changes to the wrapper based on my previous comment:

in the python mapcalc wrapper, we need to keep the seed option (for passing the seed and for backwards compatibility), so don't remove it. But seed=auto could be ignored because the new r.mapcalc autoseeding would kick in. Also we then don't need passing the flags.

[petrasovaa]

@y-sudharshan could you check why the tests are failing and try to fix it? The r.mapcalc autoseeding is now merged so the CI is already running with the new r.mapcalc code. Note that you have to check for the seed='auto' for backwards compatibility, it can't be passed to r.mapcalc, that may the source of some issues here.

[y-sudharshan]

Hi, please rerun the tests. The test failed due to a network issue when trying to install the R.Lake series extension with g.extension

[echoix]

Hi, please rerun the tests. The test failed due to a network issue when trying to install the R.Lake series extension with g.extension

Done

[y-sudharshan]

Hi, just following up on the PR.
Whenever you get a chance, could you please review it and let me know if any changes are required? Thanks!

[petrasovaa]

Thanks for your patience, coming back from holidays...

The test is failing during the cleanup, you just hid that in the try except block. Look at some other pytest fixtures, e.g. r.mask test, they use temporary directories, so cleanup is automatic.

As for the tests, I would simplify those, they should run quickly. You can use grass.script.array if you want to test the values.

[y-sudharshan]

Hi, could you please re-run the workflow? It should pass on the next run.
Thanks!

[petrasovaa]

Looks good now! Thank you for your contribution!

[y-sudharshan]

Looks good now! Thank you for your contribution!

Thank you for taking the time to review this PR! I appreciate your feedback and insights .