-
-
Notifications
You must be signed in to change notification settings - Fork 323
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
t.rast.univar/t.rast3d.univar: add support for multiprocessing #2624
Conversation
I am unable to test this at this time, but on quick review, it looks
like a reasonable solution. I would encourage those who can, please test
this. Temporal modules should greatly benefit from parallelization.
Also note that modern processors can process two threads per core.
Results of timed tests from nprocs to 2*nprocs may or may not be
interesting.
On 11/3/2022 4:49 PM, Stefan Blumentrath wrote:
This PR adds support for multiprocessing to
|python/grass/temporal/univar_statistics.py| and ultimately to
|t.rast.univar| and |t.rast3d.univar|.
A parallel testcase is added. However, thorough testing would be great!
Fixes #2590 <#2590>
------------------------------------------------------------------------
You can view, comment on, or merge this pull request online at:
#2624
Commit Summary
* d4abf62
<d4abf62>
add multiprocessing
* da34cfc
<da34cfc>
add multiprocessing
* f1cc6d3
<f1cc6d3>
add multiprocessing
* 36037e6
<36037e6>
add multiprocessing
* a2f7064
<a2f7064>
add multiprocessing
File Changes
(5 files <https://github.com/OSGeo/grass/pull/2624/files>)
* *M* python/grass/temporal/univar_statistics.py
<https://github.com/OSGeo/grass/pull/2624/files#diff-04627d74bb071d406045cfb19f3b3da489a7ad17e154d1bfbf6c0dbd6f085f84>
(180)
* *M* temporal/t.rast.univar/t.rast.univar.py
<https://github.com/OSGeo/grass/pull/2624/files#diff-4a746bd5e64a26e041d21547bdd2e54af8a237eb04b05c19f9a5a370600f5249>
(7)
* *M* temporal/t.rast.univar/testsuite/test_t_rast_univar.py
<https://github.com/OSGeo/grass/pull/2624/files#diff-c242aba237b81a17332111605d5890493607427db94e008c15a2df550cbae275>
(27)
* *M* temporal/t.rast3d.univar/t.rast3d.univar.py
<https://github.com/OSGeo/grass/pull/2624/files#diff-52754d00fb4bff367dc68c534cc885172bc2432cdbaed18a3b66bff615719a5b>
(7)
* *M* temporal/t.rast3d.univar/testsuite/test_t_rast3d_univar.py
<https://github.com/OSGeo/grass/pull/2624/files#diff-645d893ca58fae6d5a330d7ff36019f6b3c177d0f75600b4b2f899dd09880849>
(37)
Patch Links:
* https://github.com/OSGeo/grass/pull/2624.patch
* https://github.com/OSGeo/grass/pull/2624.diff
—
Reply to this email directly, view it on GitHub
<#2624>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AQZTHVI2ALJ2O5NHLL2GICTWGRFRPANCNFSM6AAAAAARWVKDOQ>.
You are receiving this because you are subscribed to this
thread.Message ID: ***@***.***>
--
Best Regards,
-Brad
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I have just a few small comments.
|
||
############################################################################### | ||
|
||
|
||
def compute_univar_stats(row, stats_module, fs, rast_region=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand where the row came from, but it is rather weird as a function parameter name...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parameter name and documentation updated in: 12eec61
def compute_univar_stats(row, stats_module, fs, rast_region=False): | ||
"""Compute univariate statistics for a map of a space time raster or raster3d dataset | ||
|
||
:param row: Must be "strds" or "str3ds" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like is must string "strds" or "str3ds", so probably just drop the quotes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated docstring in 12eec61
pool.close() | ||
pool.join() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe these are not doing anything here, close and join would be useful for starmap_async.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted in: 12eec61
…#2624) * add multiprocessing * implement suggestions from review
…#2624) * add multiprocessing * implement suggestions from review
…#2624) * add multiprocessing * implement suggestions from review
This PR adds support for multiprocessing to
python/grass/temporal/univar_statistics.py
and ultimately tot.rast.univar
andt.rast3d.univar
.A parallel testcase is added. However, thorough testing would be great!
Fixes #2590