Skip to content

r3.univar: Bring r3.univar implementation closer to r.univar #5939

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 13 commits into from
Jul 3, 2025

Conversation

echoix
Copy link
Member

@echoix echoix commented Jun 22, 2025

This isn't much, but I happened to see when looking why there were some differences in the strings used between r and r3 versions of some code, that the implementations were more diverging than only what is needed for handling the third dimension.

I had the two files compared, side by side, and with moved blocks detection enabled to see.
Some parts of r3 had more understandable variable names, whilst the r version had gui section tags that I copied over to the r3 version.

There was a static function "univar_stat_with_percentiles" in r.univar that I would've like to use as is in the r3 version, but since it is "static", it doesn't work to define it in another file. Is duplicating that function the correct approach?

I've read about the sizeof(type) vs sizeof expression, and kinda concluded that it doesn't really matter to parenthesize an expression. I verified a sample little program on Godbolt: https://godbolt.org/z/Wc31Pboqx

@github-actions github-actions bot added raster Related to raster data processing C Related code is in C libraries module labels Jun 22, 2025
@echoix
Copy link
Member Author

echoix commented Jun 27, 2025

Solved conflicts

Copy link
Contributor

@petrasovaa petrasovaa left a comment

Choose a reason for hiding this comment

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

Looking at it again, I would move the function to stats.c, no?

@echoix
Copy link
Member Author

echoix commented Jul 2, 2025

I tried, but a static function doesn't show up to other files. It was getting harder and I didn't feel comfortable (as I couldn't evaluate the impacts), of changing the header file or not being static.

You're free to adapt this as you'd like. The function in the 2D and 3D version is either exactly the same, or almost the same. I opened both in comparison mode (with moved code detection) to see what diverged.

@petrasovaa
Copy link
Contributor

I think you should move this to stats.c without the "static", and add it to globals.h. I don't see problem with that.

@echoix
Copy link
Member Author

echoix commented Jul 2, 2025

And is globals.h becoming something part of the "public" api? I imagine it should be something available only for the implementation of the two files that use it.

@echoix echoix requested a review from petrasovaa July 2, 2025 21:20
@echoix
Copy link
Member Author

echoix commented Jul 2, 2025

Looking at it again, I would move the function to stats.c, no?

I think you should move this to stats.c without the "static", and add it to globals.h. I don't see problem with that.

@petrasovaa A bit like this? It should work?

@echoix
Copy link
Member Author

echoix commented Jul 2, 2025

The remaining big differences between r.univar vs r3.univar:

  • r.univar has OpenMP support.
  • r.univar has nprocs parameter
  • r.univar has an "r" flag: "Use the native resolution and extent of the raster map, instead of the current region"
  • r.univar uses kahan sum (kahan_sum defined in that file)
  • r.univar seems to have a loop to count input rasters, but r3.univar handles only one (see standard options G_OPT_R_MAPS vs G_OPT_R3_MAP)
  • r.univar has open_raster and process_raster functions, some of it seems related to having OpenMP code more localized in process_raster

@echoix
Copy link
Member Author

echoix commented Jul 2, 2025

I tried, but a static function doesn't show up to other files. It was getting harder and I didn't feel comfortable (as I couldn't evaluate the impacts), of changing the header file or not being static.

You're free to adapt this as you'd like. The function in the 2D and 3D version is either exactly the same, or almost the same. I opened both in comparison mode (with moved code detection) to see what diverged.

The function univar_stat_with_percentiles was exactly the same between implementations (in r3.univar, it was added in a previous version of this PR, not on main yet)

@nilason
Copy link
Contributor

nilason commented Jul 3, 2025

I've read about the sizeof(type) vs sizeof expression, and kinda concluded that it doesn't really matter to parenthesize an expression.

Parentheses are required with types, as in sizeof(int) or sizeof(char), otherwise (variables) they are not. I always use it for clarity.

And is globals.h becoming something part of the "public" api? I imagine it should be something available only for the implementation of the two files that use it.

The modals are all non-public (end up in executables only), no header files are distributed. So, in this case the globals.h is available for all source files (that include it) in the raster/r.univar directory. Note in Makefile:

r_univar_OBJS = r.univar_main.o sort.o stats.o
r3_univar_OBJS = r3.univar_main.o sort.o stats.o

the stats.c and sort.c file are compiled with both r.univar and r3.univar.

@nilason nilason added this to the 8.5.0 milestone Jul 3, 2025
@echoix
Copy link
Member Author

echoix commented Jul 3, 2025

Thanks for the theory, I appreciate it!

@echoix echoix enabled auto-merge (squash) July 3, 2025 12:00
@echoix echoix dismissed petrasovaa’s stale review July 3, 2025 12:01

Addressed and approved by another after that?

@echoix echoix merged commit 8dbe947 into OSGeo:main Jul 3, 2025
27 checks passed
@echoix echoix deleted the r3.univar-sync-with-r.univar branch July 3, 2025 12:01
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 raster Related to raster data processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants