r.mfilter: implement parallelization with OpenMP #1708

aaronsms · 2021-07-04T10:19:35Z

Checklists before merging:

aaronsms · 2021-07-05T09:23:01Z

The previous workflow run has all passsing checks, except for OSGeo4W, where the download link is no longer available.

marisn

Code looks OK. I only could not run some tests as I don't have "slope" map – would be nice to replace with one present in the basic version of NC location.

marisn · 2021-07-10T14:22:15Z

raster/r.mfilter/testsuite/test_r_mfilter.py

+        filter = self.create_filter(self.filter_options["sequential"])
+        self.assertModule(
+            "r.mfilter",
+            input="slope",


My version of nc_basic_spm_grass7 location does not have a slope map. Can it be replaced with something else to allow testing on a basic location?

@aaronsms Slope is missing in the basic location. It can be a good idea to use the basic location for writing test scripts.

@marisn Any specific reasons why you don't (cannot?) use the full nc_spm_08_grass7?

I will replace 'slope' with 'lakes'.

Why not elevation?

petrasovaa · 2021-07-12T15:20:35Z

I tested it with bigger data and it crashes with Error reading temporary file. I run it on a slope map (derived from US 30m DEM) and it shows up with region with
16.832.104.560 cells and nprocs=1 and with 900.000.000 cells and nprocs=2. I ran a benchmark with this data successfully for 1.000.000 cells.
I also ran it with the original version of r.mfilter and that works fine.

HuidaeCho · 2021-07-14T02:47:23Z

16.832.104.560 cells and nprocs=1

@aaronsms Please check nprocs=1 code again. In this case, I think we need to revert back to the original behavior for nprocs=1. I thought we're not creating more tempfiles for r.mfilter, but are they bigger now even for nrpcos=1?

petrasovaa · 2021-07-14T03:25:30Z

Also, I ran it with valgrind and it gave me one error, and although it didn't look too related as far as I remember, it's worth checking that.

aaronsms · 2021-07-14T15:20:15Z

I thought we're not creating more tempfiles for r.mfilter, but are they bigger now even for npcos=1?

For r.mfilter, we are not changing the default behavior. The original one uses a temporary file as buffer as well.

it shows up with region with
16.832.104.560 cells and nprocs=1 and with 900.000.000 cells and nprocs=2

@petrasovaa, can I clarify that whether you meant, it works with the mentioned two cases or, you achieve different results with (nprocs = 1 vs = 2)? Also, may I also ask at what cell count did it crash with the Error reading temporary files? Thanks

aaronsms · 2021-07-14T15:49:45Z

Currently, I have found some issues when grass is compiled without OpenMP, will follow up soon. But I haven't found issues with the one compiled with OpenMP.

petrasovaa · 2021-07-14T15:54:36Z

@petrasovaa, can I clarify that whether you meant, it works with the mentioned two cases or, you achieve different results with (nprocs = 1 vs = 2)? Also, may I also ask at what cell count did it crash with the Error reading temporary files? Thanks

16.832.104.560 cells and nprocs=1
and
900.000.000 cells and nprocs=2
failed with the Error reading temporary files

With just 1.000.000 cells everything worked, although I haven't looked at the resulting raster closely.

aaronsms · 2021-07-16T04:15:37Z

@petrasovaa I have solved the overflow issues, so it should now work for larger raster map as per previous behavior.

petrasovaa · 2021-07-17T19:23:37Z

Here is my benchmark on a slope map of 16.000.000.000 cells and 5x5 filter, only 3 repeats, on a machine with 28 cores:

petrasovaa · 2021-07-24T02:42:30Z

Another benchmark, this time with larger filter (15x15), the rest is the same (slope map of 16.000.000.000 cells, 3 repeats, on a machine with 28 cores):

aaronsms · 2021-07-25T11:04:25Z

Another benchmark, this time with larger filter (15x15), the rest is the same (slope map of 16.000.000.000 cells, 3 repeats, on a machine with 28 cores):

Hi @petrasovaa, I was wondering if I can make use of this benchmark result for the twitter post to show the result on large map? Thanks.

petrasovaa · 2022-01-06T15:30:46Z

Question, I can see that in PR #1724 (r.neighbors) you use memory option but not here. Also I can see huge temporary file being created when I run this on large data. @aaronsms are there any improvements in the r.neighbors PR that would be relevant here?

aaronsms · 2022-01-07T03:19:56Z

Question, I can see that in PR #1724 (r.neighbors) you use memory option but not here. Also I can see huge temporary file being created when I run this on large data. @aaronsms are there any improvements in the r.neighbors PR that would be relevant here?

For r.mfilter, the original behavior is we use at most 2 temp files for processes requiring 2 or more sequential filters. This PR maintains that behavior of using temp files.

I think there can be improvements from r.neighbor that would be relevant here (similar approach but will change the current behavior), e.g., if the users want to use memory instead of disk. For example, if a chunk of memory is used, then we can propagate from start all the way to finish, before moving on to the next chunk. Could be worth exploring, then it would remove the need for temp files altogether.

petrasovaa · 2022-01-07T04:36:26Z

Thanks for replying, now I remember I ran into this before. Since this was the original behavior, I plan to merge this as it is now.

petrasovaa · 2022-01-10T18:24:32Z

Tested again, with 16.000.000.000 cells (US digital elevation model), 2 different window sizes, 3 repeats:

This is result of the benchmark from this PR:

petrasovaa · 2022-01-10T19:20:36Z

I updated manual page with performance section, not sure if there is something else that needs to go in there.
It would be useful to have a keyword for parallel modules, I think this was even discussed, not sure if there was any conclusion. I suggest parallel, but I am open to other options.

neteler · 2022-01-10T20:55:11Z

It would be useful to have a keyword for parallel modules, I think this was even discussed, not sure if there was any conclusion. I suggest parallel, but I am open to other options.

Very good idea! Another keyword might be multicore but parallel sounds better to me. Others?

nilason · 2022-01-10T21:00:39Z

parallel

👍

Co-authored-by: Anna Petrasova <kratochanna@gmail.com>

aaronsms force-pushed the parallel-r.mfilter branch from c5911b9 to d11db3c Compare July 5, 2021 09:10

wenzeslaus added this to In progress in Raster Parallelization GSoC 2021 via automation Jul 6, 2021

wenzeslaus added enhancement New feature or request gsoc Reserved for Google Summer of Code student(s) labels Jul 6, 2021

aaronsms force-pushed the parallel-r.mfilter branch from d11db3c to 74eb924 Compare July 9, 2021 09:05

marisn approved these changes Jul 10, 2021

View reviewed changes

aaronsms force-pushed the parallel-r.mfilter branch from 74eb924 to 5ff8b6b Compare July 14, 2021 14:36

aaronsms force-pushed the parallel-r.mfilter branch from 1b95fdf to 05cca82 Compare July 15, 2021 06:17

aaronsms force-pushed the parallel-r.mfilter branch from 05cca82 to 035390c Compare July 16, 2021 16:06

HuidaeCho added the raster Related to raster data processing label Aug 1, 2021

aaronsms changed the title ~~r.mfilter: Add parallel support and test cases~~ r.mfilter: implement parallelization with OpenMP Aug 4, 2021

aaronsms force-pushed the parallel-r.mfilter branch from 3f12285 to 84f1ca5 Compare August 13, 2021 12:33

aaronsms force-pushed the parallel-r.mfilter branch from 84f1ca5 to 3dfdf0e Compare August 20, 2021 12:50

wenzeslaus added this to the 8.2.0 milestone Aug 24, 2021

neteler mentioned this pull request Oct 10, 2021

[Bug] CI error: flake8 Expected - Waiting for status to be reported #1935

Closed

aaronsms added 4 commits December 5, 2021 19:18

Add test cases

b294a25

Refactor to use pointers

6c4ac32

Add parallel implementation

a625b78

Disable parallel execution for SEQUENTIAL filter

b38ca24

aaronsms added 8 commits December 5, 2021 19:18

Add repeated filters test case

533442a

Benchmark mfilter with generated fractal

2dd8aa0

Change slope to using lakes map

f0d2953

Fix private variables in non-OpenMP compilation

9f34c9f

Solve size_t overflow issues

d216972

Handle errors on writing temporary files

588175f

Add OpenMP compilation warning

2a1a341

Allow r.random.surf to fail for benchmarking

9b1e32d

aaronsms force-pushed the parallel-r.mfilter branch from 3dfdf0e to 9b1e32d Compare December 5, 2021 11:18

add performance section to manual

486a731

petrasovaa approved these changes Jan 10, 2022

View reviewed changes

petrasovaa added 2 commits January 12, 2022 11:10

add parallel keyword, add aaron to authors

b09420d

standardize level of headings in manual

10eadef

petrasovaa merged commit 829768d into OSGeo:main Jan 12, 2022

Raster Parallelization GSoC 2021 automation moved this from In progress to Done Jan 12, 2022

ninsbl pushed a commit to ninsbl/grass that referenced this pull request Oct 26, 2022

r.mfilter: implement parallelization with OpenMP (OSGeo#1708)

010b1ad

Co-authored-by: Anna Petrasova <kratochanna@gmail.com>

ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023

r.mfilter: implement parallelization with OpenMP (OSGeo#1708)

827810b

Co-authored-by: Anna Petrasova <kratochanna@gmail.com>

neteler pushed a commit to nilason/grass that referenced this pull request Nov 7, 2023

r.mfilter: implement parallelization with OpenMP (OSGeo#1708)

d871b0b

Co-authored-by: Anna Petrasova <kratochanna@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

r.mfilter: implement parallelization with OpenMP #1708

r.mfilter: implement parallelization with OpenMP #1708

aaronsms commented Jul 4, 2021 •

edited by petrasovaa

Loading

aaronsms commented Jul 5, 2021

marisn left a comment

marisn Jul 10, 2021

HuidaeCho Jul 14, 2021 •

edited

Loading

aaronsms Jul 14, 2021

petrasovaa Jul 14, 2021

petrasovaa commented Jul 12, 2021

HuidaeCho commented Jul 14, 2021

petrasovaa commented Jul 14, 2021

aaronsms commented Jul 14, 2021 •

edited

Loading

aaronsms commented Jul 14, 2021

petrasovaa commented Jul 14, 2021

aaronsms commented Jul 16, 2021

petrasovaa commented Jul 17, 2021

petrasovaa commented Jul 24, 2021 •

edited by wenzeslaus

Loading

aaronsms commented Jul 25, 2021

petrasovaa commented Jan 6, 2022

aaronsms commented Jan 7, 2022 •

edited

Loading

petrasovaa commented Jan 7, 2022

petrasovaa commented Jan 10, 2022

petrasovaa commented Jan 10, 2022

neteler commented Jan 10, 2022

nilason commented Jan 10, 2022

r.mfilter: implement parallelization with OpenMP #1708

r.mfilter: implement parallelization with OpenMP #1708

Conversation

aaronsms commented Jul 4, 2021 • edited by petrasovaa Loading

aaronsms commented Jul 5, 2021

marisn left a comment

Choose a reason for hiding this comment

marisn Jul 10, 2021

Choose a reason for hiding this comment

HuidaeCho Jul 14, 2021 • edited Loading

Choose a reason for hiding this comment

aaronsms Jul 14, 2021

Choose a reason for hiding this comment

petrasovaa Jul 14, 2021

Choose a reason for hiding this comment

petrasovaa commented Jul 12, 2021

HuidaeCho commented Jul 14, 2021

petrasovaa commented Jul 14, 2021

aaronsms commented Jul 14, 2021 • edited Loading

aaronsms commented Jul 14, 2021

petrasovaa commented Jul 14, 2021

aaronsms commented Jul 16, 2021

petrasovaa commented Jul 17, 2021

petrasovaa commented Jul 24, 2021 • edited by wenzeslaus Loading

aaronsms commented Jul 25, 2021

petrasovaa commented Jan 6, 2022

aaronsms commented Jan 7, 2022 • edited Loading

petrasovaa commented Jan 7, 2022

petrasovaa commented Jan 10, 2022

petrasovaa commented Jan 10, 2022

neteler commented Jan 10, 2022

nilason commented Jan 10, 2022

aaronsms commented Jul 4, 2021 •

edited by petrasovaa

Loading

HuidaeCho Jul 14, 2021 •

edited

Loading

aaronsms commented Jul 14, 2021 •

edited

Loading

petrasovaa commented Jul 24, 2021 •

edited by wenzeslaus

Loading

aaronsms commented Jan 7, 2022 •

edited

Loading