Skip to content

r.mapcalc: remove static variables from mode and median to fix parallelization #6087

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 21, 2025

Conversation

petrasovaa
Copy link
Contributor

This is an attempt to get rid of static variables from the r.mapcalc functions for mode and median. The static variables were there I assume to not allocate for every row, but it complicates parallelization, see #5742. Since the size of the array is typically very small (the number of arguments of the median() or mode() function, can be raster or number), it will use array on stack and allocate only for larger sizes.

I am not a C expert, but hopefully this makes sense.

@github-actions github-actions bot added raster Related to raster data processing Python Related code is in Python C Related code is in C libraries module tests Related to Test Suite labels Jul 19, 2025
Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I consider this a good tradeoff. It may be slower, but it will reliably work with parallelization. If it is too slow, we can optimize by creating a context structure which would be unique for each thread.

The compilation is broken given the more strict requirement in the CI on C standard.

Copy link
Contributor

@nilason nilason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes sense. It might even be faster for cases within the threshold (using heap allocated memory instead of realloc), that being without the additional positive prospects of parallelisation...

Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

As for the f-strings, I really meant just the short ones in the setup, the remaining ones are matter of taste, but may trigger some linters.

@wenzeslaus wenzeslaus merged commit 4131a76 into OSGeo:main Jul 21, 2025
27 checks passed
@github-actions github-actions bot added this to the 8.5.0 milestone Jul 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C Related code is in C libraries module Python Related code is in Python raster Related to raster data processing tests Related to Test Suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants