-
Notifications
You must be signed in to change notification settings - Fork 230
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: Add basic2 mode #2307
base: master
Are you sure you want to change the base?
mpi: Add basic2 mode #2307
Conversation
024e82b
to
b4e67e3
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2307 +/- ##
========================================
Coverage 87.35% 87.36%
========================================
Files 238 238
Lines 45574 45702 +128
Branches 4035 4053 +18
========================================
+ Hits 39813 39927 +114
- Misses 5079 5092 +13
- Partials 682 683 +1 ☔ View full report in Codecov by Sentry. |
befd078
to
89387c8
Compare
ff919e2
to
895cef3
Compare
895cef3
to
51411cc
Compare
|
||
return SendRecv('sendrecv%s' % key, iet, parameters, bufg, bufs) | ||
|
||
def _call_sendrecv(self, name, *args, msg=None, haloid=None): |
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.
why do you need a haloid
here ? never had to use it. What makes this mode require it?
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.
Similarly to the same method in OverlapHalo. Each call to sendrecv has its own message indexedpointer
mapper[(d0, side, region)] = (sizes) | ||
|
||
i = 0 | ||
for d in f.dimensions: |
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.
why not :
for i, halo in enumerate(self.halos):
like we have in the other message types?
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 followed the basic style, which does not have yet a method to cleanup the redundant halos as in diag2 for example:
e.g.:
# Only retain the halos required by the Diag scheme
halos = sorted(i for i in hse.halos if isinstance(i.dim, tuple))
I tried this but did not manage to get it working nicely.
51411cc
to
6a0ce49
Compare
6a0ce49
to
7d206db
Compare
tests/test_mpi.py
Outdated
@@ -801,7 +801,7 @@ def test_trivial_eq_2d(self): | |||
assert np.all(f.data_ro_domain[0, :-1, -1:] == side) | |||
assert np.all(f.data_ro_domain[0, -1:, :-1] == side) | |||
|
|||
@pytest.mark.parallel(mode=[(8, 'basic'), (8, 'diag'), (8, 'overlap'), | |||
@pytest.mark.parallel(mode=[(8, 'basic'), (8, 'basic2'), (8, 'diag'), (8, 'overlap'), |
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 would drop basic
here and below... or overlap
perhaps. These tests can be quite expensive
5a5e826
to
c266e14
Compare
e849e40
to
55b11d5
Compare
@@ -1185,6 +1297,16 @@ def _as_number(self, v, args): | |||
assert args is not None | |||
return int(subs_op_args(v, args)) | |||
|
|||
def _allocate_buffers(self, f, shape, entry): |
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 could use some whitespace to make it more readable
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.
agree, added a docstring as well
58dbb96
to
dab37fd
Compare
dab37fd
to
12f133c
Compare
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.
Is documentation required somewhere to explain what the new Basic2
mode does?
01ed5e2
to
28825e1
Compare
mpi: Drop redundant determinism
a54684a
to
834a627
Compare
you are right, will add! |
834a627
to
a94b1d5
Compare
@EdCaunt I just remember that I have some enhanced docs on another branch, so this can be fine for the moment! |
Preallocated buffers using MPIMsg.
An MPIMsgEnriched version for send/recv, will follow