mpi data gatherer #1376
mpi data gatherer #1376
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1376 +/- ##
===========================================
- Coverage 86.27% 76.14% -10.14%
===========================================
Files 193 187 -6
Lines 27777 27432 -345
Branches 3758 3741 -17
===========================================
- Hits 23965 20888 -3077
- Misses 3387 6075 +2688
- Partials 425 469 +44
Continue to review full report at Codecov.
|
devito/data/data.py
Outdated
| @@ -174,9 +175,9 @@ def __str__(self): | |||
| return super(Data, self._local).__str__() | |||
|
|
|||
| @_check_idx | |||
| def __getitem__(self, glb_idx, comm_type): | |||
| def __getitem__(self, glb_idx, comm_type, _gather=None): | |||
There was a problem hiding this comment.
variable name == method name ?
There was a problem hiding this comment.
Yeah, the whole variable possibly needs an overhaul.
So I'm thinking should it be allowed to be a tuple? i.e. you can gather simultaneously on multiple ranks. I'm not sure how often users would want this though + it adds a decent bit of complexity to the routine to save users (probably?) a couple of lines of code.
So would only be worth it if users are going to frequently do something along the lines of
a = f.data_gather(rank='all')
devito/data/data.py
Outdated
| local_dat = recval.wait() | ||
| loc_ind = local_si[local_dat[0]] | ||
| retval.data[loc_ind] = local_dat[1] | ||
| # TODO: These statements can potentially be fused? |
There was a problem hiding this comment.
yeah I can see there's some overlap... also the while-if-elif-else structure is identical AFAICT. We might want to abstract it away into a separate routine which takes callbacks (the bodies each each if-branch) as input? or is it just painful over-engineering?
in the future, we might want to add other gather-like operations -- scatter, broadcast, ...
There was a problem hiding this comment.
Yes, it is identical at the moment. It possibly should stay identical depending on what functionality we want to allow - but we can discuss that.
We might want to abstract it away into a separate routine
Yes, we definitely want to do that - it will make the fusion much cleaner. I'm just keeping them separate for the time being until I get all required functionality working with the gather, after that I'll have a good idea re. the best way to fuse them.
rhodrin
force-pushed
the
data_gather
branch
from
d22725d to
0268931
Compare
devito/data/data.py
Outdated
| def _gather(self, start=None, stop=None, step=1, rank=0): | ||
| """ | ||
| Method for gathering distributed data into a NumPy array | ||
| on a single rank. See the the public ``data_gather`` method |
devito/types/dense.py
Outdated
| The final point of the `slice` to include. | ||
| Notes | ||
| ----- | ||
| Notes will go here. |
There was a problem hiding this comment.
TODO
maybe also add something along the lines of "users should be aware that calling this method might blow up the memory since... so use it judiciously" -- what do you say?
rhodrin
force-pushed
the
data_gather
branch
from
d131d2c to
9750d51
Compare
devito/data/data.py
Outdated
| loc_idx = self._index_glb_to_loc(glb_idx) | ||
| if comm_type is index_by_index: | ||
| if comm_type is index_by_index or isinstance(gather, int): |
There was a problem hiding this comment.
Should we have a default gather=True equivalent to gather=0? Or is it overkill
devito/data/data.py
Outdated
| loc_idx = self._index_glb_to_loc(glb_idx) | ||
| if comm_type is index_by_index: | ||
| if comm_type is index_by_index or isinstance(gather, int): |
There was a problem hiding this comment.
Also there is a lot of isinstance(gather, int), wouldn't it be nicer to have gather = int(gather) if isinstance(gather, int) else None at the beginning and then only needs if gather everywhere below. The int(gather) probably not necessary actually but just here for safety.
There was a problem hiding this comment.
gather can be, and often is, 0 which is why is instance isinstance is used. But we could have the kwarg be gather_rank and then introduce gather as a True/False - leads to one more line of code, but code is then a bit tidier I guess.
tests/test_data.py
Outdated
| @pytest.mark.parallel(mode=4) | ||
| @pytest.mark.parametrize('rank', [0, 1, 2, 3]) | ||
| def test_simple_gather(self, rank): | ||
| """ Fill me in.""" |
There was a problem hiding this comment.
Thanks. the 3 instances of it will be fixed on next push.
| loc_idx = self._index_glb_to_loc(glb_idx) | ||
| if comm_type is index_by_index: | ||
| gather = True if isinstance(gather_rank, int) else False |
There was a problem hiding this comment.
nitpicking for code homogeneity, perhaps for another PR: typically we call these variables "is_..." in this case "is_gather". The "is_" encodes the information that the variable is a boolean
| (1, 8, 3), | ||
| ((0, 4, 4), None, (2, 1, 1))]) | ||
| def test_sliced_gather_3D(self, start, stop, step): | ||
| """ Test gather for various 2D slices.""" |
There was a problem hiding this comment.
oh, bleh, thanks. I'll add this and the thing @FabioLuporini mentioned to #1457.
This PR adds a method to
Function's to enable the gathering of distributedDataonto a single rank.NOTE: PR'ing for CI spin - needs lots of clean-up, fixes and many more tests. Will update here with more details when appropriate.