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.resamp.filter: implement parallelization with OpenMP #1759

Merged
merged 12 commits into from
Jan 4, 2023

Conversation

aaronsms
Copy link
Contributor

@aaronsms aaronsms commented Jul 23, 2021

Checklists before merging:

  • code review
  • CI passes
  • performance section in documentation
  • confirm values in test are from the old version (run new tests with old code)
  • run tests without OpenMP
  • visual check of results with custom data ("looks good" with non NC SPM dataset)
  • check that it works with really large data (16B cells)
  • run multi-core benchmark (no degraded performance with many threads)
  • run one core benchmark on many resolutions or many cell
  • rebase to main

@wenzeslaus wenzeslaus added enhancement New feature or request gsoc Reserved for Google Summer of Code student(s) labels Jul 24, 2021
@HuidaeCho HuidaeCho added the raster Related to raster data processing label Aug 1, 2021
Copy link
Contributor

@marisn marisn left a comment

Choose a reason for hiding this comment

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

Didn't test the performance, but the code and generated output looks good.

static DCELL **bufs;
static DCELL ***bufs;
static int bufrows;
static int nprocs;
Copy link
Member

Choose a reason for hiding this comment

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

Delete this line if not used.

main.c:169:12: warning: ‘nprocs’ defined but not used [-Wunused-variable]
 static int nprocs;

} parm;
struct
{
struct Flag *nulls;
} flag;
char title[64];
int i;
int i, t;
int nprocs;
Copy link
Member

Choose a reason for hiding this comment

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

Or did you mean to use the static nprocs above?

@aaronsms aaronsms force-pushed the parallel-r.resamp.filter branch 2 times, most recently from 2c9f530 to 46c607d Compare August 20, 2021 12:52
@wenzeslaus wenzeslaus added this to the 8.2.0 milestone Aug 24, 2021
@wenzeslaus wenzeslaus modified the milestones: 8.2.0, 8.4.0 Mar 16, 2022
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.

Code looks good to me and test succeeds on Mac.
Great!

Minor suggestion though:

--- a/raster/r.resamp.filter/main.c
+++ b/raster/r.resamp.filter/main.c
@@ -107,7 +107,7 @@ static const struct filter_type menu[] = {
     {"hann", f_hann, 0},
     {"hamming", f_hamming, 0},
     {"blackman", f_blackman, 0},
-    {NULL},
+    {NULL, NULL, 0},
 };

@petrasovaa
Copy link
Contributor

Thank you for the review, I will merge now. #2692 needs to be again updated.

@petrasovaa petrasovaa merged commit 3f5cd95 into OSGeo:main Jan 4, 2023
@nilason
Copy link
Contributor

nilason commented Jan 4, 2023

Thank you for the review, I will merge now. #2692 needs to be again updated.

Done.

ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023
neteler pushed a commit to nilason/grass that referenced this pull request Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request gsoc Reserved for Google Summer of Code student(s) raster Related to raster data processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants