Skip to content
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

r.sim: cleanup of global variables #5044

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

petrasovaa
Copy link
Contributor

This is an effort to get clean up the r.sim code:

  • group global variables into structures
  • remove unused code
  • limit scope of some variables
  • fix small bug in parallelization code (nwalka variable)

There will be more changes/fixes, but perhaps it would be good to get this merged first.

@petrasovaa petrasovaa added this to the 8.5.0 milestone Feb 4, 2025
@petrasovaa petrasovaa requested a review from wenzeslaus February 4, 2025 17:03
@github-actions github-actions bot added raster Related to raster data processing C Related code is in C module labels Feb 4, 2025
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 love these cleanups, so much readable and manageable code, thanks!
I only left some minor suggestions.

scan-build reported a number of issues. Although they are probably not introduced by this PR, I attached the report (html bundle) here for easy check:

scan-build-2025-02-05-105358-93516-1.zip

raster/r.sim/r.sim.sediment/main.c Outdated Show resolved Hide resolved
raster/r.sim/simlib/simlib.h Outdated Show resolved Hide resolved
raster/r.sim/r.sim.water/main.c Outdated Show resolved Hide resolved
raster/r.sim/simlib/simlib.h Outdated Show resolved Hide resolved
raster/r.sim/simlib/simlib.h Show resolved Hide resolved
@petrasovaa
Copy link
Contributor Author

I love these cleanups, so much readable and manageable code, thanks! I only left some minor suggestions.

scan-build reported a number of issues. Although they are probably not introduced by this PR, I attached the report (html bundle) here for easy check:

scan-build-2025-02-05-105358-93516-1.zip

Thanks! I will look at it, maybe address some of these in a later PR.

@petrasovaa petrasovaa requested a review from cwhite911 February 10, 2025 17:39
@petrasovaa petrasovaa enabled auto-merge (squash) February 10, 2025 19:24
Copy link
Contributor

@cwhite911 cwhite911 left a comment

Choose a reason for hiding this comment

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

@petrasovaa are you planning on removing waterglobs.h? If not, add a comment in the file explaining it's purpose and add nice comment's to the obscure variables names.

int maxwab;
double step, conv;
typedef struct {
double halpha;
Copy link
Contributor

Choose a reason for hiding this comment

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

Add comments explaining what these variables are.

struct WaterParams {
double xmin, ymin, xmax, ymax;
double mayy, miyy, maxx, mixx;
typedef struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add some comments here to document what this geometry struct is design to do.

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 module raster Related to raster data processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants