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
mpi: Mitigate SparseFunction setup costs #1720
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1720 +/- ##
==========================================
- Coverage 89.61% 86.93% -2.69%
==========================================
Files 206 201 -5
Lines 33216 32655 -561
Branches 4314 4277 -37
==========================================
- Hits 29766 28388 -1378
- Misses 2960 3758 +798
- Partials 490 509 +19
Continue to review full report at Codecov.
|
02e2326
to
bf680e0
Compare
16ef69d
to
4388657
Compare
devito/mpi/distributed.py
Outdated
return as_tuple(ret) | ||
elif sum(index.shape) == 1: | ||
return index | ||
print(index.shape, self.ndim) |
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.
leftover
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.
aside from this print, is this PR basically good to go?
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.
Sorry yes did the fix locally and then forgot about pushing so gotta redo it, basically is yes. Did you give it a spin?
devito/types/sparse.py
Outdated
for r in self.grid.distributor.glb_to_rank(s): | ||
ret.setdefault(r, []).append(i) | ||
return {k: filter_ordered(v) for k, v in ret.items()} | ||
dmap = self.grid.distributor.glb_to_rank(self._support) |
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.
single line return self. ...
?
""" | ||
This method is analogous to :meth:`_dist_scatter_mask`, although | ||
the mask is now suitable to index into self's SubFunctions, rather | ||
than into ``self.data``. | ||
""" | ||
return self._dist_scatter_mask[self._sparse_position] | ||
return self._dist_scatter_mask(dmap=dmap)[self._sparse_position] |
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 is, essentially, to avoid recomputation of _dist_datamap
when processing eg coordinates?
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.
yes, it's called like 8 times so this gain quite a bit to do it only once.
devito/mpi/distributed.py
Outdated
index = np.expand_dims(index, axis=2) | ||
|
||
ret = {} | ||
|
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.
no blank line
retrieved. | ||
""" | ||
assert isinstance(index, (tuple, list)) | ||
if len(index) == 0: | ||
assert isinstance(index, np.ndarray) |
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.
Maybe this fixes the python 3.9 issue as well?
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.
possibly, where is the issue for it again?
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 think no issue was opened, need to check
4388657
to
5b3d994
Compare
API: make SubFunction hashable
5b3d994
to
65d5097
Compare
Some better use of numpy broadcasting to setup sparse functions
I get a ~X5 speedup on 256*256 receivers on my desktop.