-
Notifications
You must be signed in to change notification settings - Fork 3
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
Repeating a Neighbor in a Graph Communicator Ambiguity with MPI_GRAPH_NEIGHBOR[_COUNT] #3
Comments
Originally by gropp on 2008-10-26 09:43:48 -0500 The October 2008 meeting asked that the proposed text be changed to make even clearer that the same values that were passed to Graph_create are returned by graph neighbor and graph neighbor count, even if those arguments specified duplicates. |
Originally by jsquyres on 2008-12-06 11:19:20 -0600 Updated proposal text per Oct 2008 meeting. Attached a PDF so that you can see the proposed changes in the context of the rest of the chapter text. Needs reviews:
|
Originally by bronis on 2008-12-06 13:00:09 -0600 There was an error in the example. Task 0 should return 3 for MPI_GRAPH_NEIGHBORS_COUNT but the example had 2. I fixed it on Replying to [#3 jsquyres]:
|
Originally by jsquyres on 2008-12-06 14:05:14 -0600 Thanks for the catch. I'll update the PDF in a bit. |
Originally by jsquyres on 2008-12-08 16:36:12 -0600 PDF updated. |
Originally by gropp on 2008-12-09 06:44:24 -0600 Review: Why remove lines 660-664 from the topol.tex file? This seems misplaced but there is a need for this top-level text. |
Originally by jsquyres on 2008-12-09 07:02:53 -0600 Replying to gropp:
Agreed; replaced (but I moved the text down below the two functions). |
Originally by gropp on 2008-12-09 07:17:03 -0600 Thanks, that looks good. Reviewed and accepted. |
Originally by RolfRabenseifner on 2008-12-11 10:08:29 -0600 Review:
|
Originally by jsquyres on 2008-12-11 10:43:00 -0600 Replying to RolfRabenseifner:
Already done.
Good catch; thanks. Fixed.
Done.
Done. |
Originally by jsquyres on 2008-12-11 10:43:26 -0600 Attachment added: |
Originally by jsquyres on 2008-12-16 08:18:38 -0600 Reviewers -- since there were changes to this ticket, it needs to be reviewed again. Please re-submit your review. Thanks! |
Originally by bronis on 2008-12-16 08:35:46 -0600 Reviewed current version; it is fine. The question remains whether the Forum wants to reconsider the solution adopted. I am ambivalent about that. Hopefully, we can settle it once and for all today. |
Originally by gropp on 2008-12-16 08:40:11 -0600 The revision is fine with me. |
Originally by jsquyres on 2008-12-17 13:03:51 -0600
|
Originally by bronis on 2008-12-22 10:41:36 -0600 If I follow correctly, the only changes since my last review (please forgive my confusion about whether anything needed to be reconsidered here) are changes to the proposal section that do not require review and the addition of making the index entry for MPI_GRAPH_CREATE reflect the example, which is clearly correct. I think we still need two reviewers for this issue... |
Originally by jsquyres on 2008-12-23 11:05:42 -0600 Replying to bronis:
Correct on all counts. |
Originally by RolfRabenseifner on 2009-01-19 06:28:34 -0600 Okay with me. |
Originally by jsquyres on 2009-01-26 15:09:39 -0600 Bill and George -- please review so that we can discuss in San Jose. Thanks! |
Originally by gropp on 2009-02-06 14:44:01 -0600 Reviewed and still ok |
Originally by bosilca on 2009-02-09 15:31:10 -0600 I definitively prefer this solution to the one in the standard today. |
Originally by jsquyres on 2009-02-12 13:01:17 -0600 Need to ensure that Open MPI does it this way to verify the implementation. |
Originally by traff on 2009-07-13 10:07:49 -0500 Changes done by chapter author (Jesper), reviewers please see attachment |
Originally by RolfRabenseifner on 2009-07-13 12:04:03 -0500 PDF-review:
|
Originally by traff on 2009-07-14 08:03:48 -0500 Thanks Rolf,
An updated pdf has been attached Jesper |
Originally by RolfRabenseifner on 2009-07-14 11:12:50 -0500 Yes, Jesper, you are right: The sentence The neighbors array returned from MPI_GRAPH_NEIGHBORS will be edges[index[rank-1]] through edges[index[rank]]. is wrong. I checked it with the example MPI-2.1 p247:1-11. The neighbors array returned from MPI_GRAPH_NEIGHBORS will be edges[index[rank-1]] through edges[index[rank]-1]. |
Originally by traff on 2009-07-14 11:37:13 -0500 Replying to RolfRabenseifner:
eeh, yes, of course, that was, somehow what I meant... Jeff, other reviewers, can you check the contents of this? Jesper |
Originally by bronis on 2009-07-16 10:09:57 -0500 Outside the ticket system, Jesper and Rolf have sent the following email thread: Date: Thu, 16 Jul 2009 11:25:10 +0200 I fully agree with Rolf. Please have a look; I can do the edit of Jesper On Thu, Jul 16, 2009 at 11:04:52AM +0200, Rolf Rabenseifner wrote:
I have reviewed the issue and agree that it should be "edges[index[rank]-1]". I feel Bill should comment on whether this should require a vote. I believe |
Originally by RolfRabenseifner on 2009-07-16 11:16:51 -0500 I would prefere one summary ticket where all authors and reviewers add changes that have been done after 2nd vote - sorted by page/line according to MPI-2.1. Discussions should stay in the original tickets. Example: -Chap. 5 Collective ...* MPI-2.1 p158:5-7 and p158:17-19, Ticket #93: Same text as modified MPI-2.2 text for p156:15-17 and p156:25-27. -Reason:* Consistency MPI-2.1 p236:48, Ticket #49: Sentence "In C, ..." started on a new paragraph. -Reason:* better formatting, bad readability due to otherwise unfilled lines due to long constant names at end of line. -Chap. 7. Toplogies* MPI-2.1 p250:45, Ticket #42: Solution was not modified, but Extended Scope "MPI-2.1 Errata" was removed. -Reason:* to keep the MPI-2.1 Errata page small (only one page). MPI-2.1 p252:5, end of new text from Ticket #3): "edges[index[rank-1]] to edges[index[rank]-1]" instead of[[BR]] -Reason:* Without "-1", the text would be wrong. -Chap. 11. One-sided* MPI-2.1 p323:38-39 and Ticket #53: The sentence "Implementors should document ..." is kept as a own paragraph -Reason:* The concatenation of the two paragraphs may not be intented (That is all that I have been somehow involved) |
Originally by gropp on 2009-07-16 15:32:24 -0500 I view the fix for edges[rank-1] etc. to be a simple correction; further, the correction is essential because the text on which we voted was clearly wrong (though not clearly enough for the reviewers :) ). This change does not require a vote (other than the final votes on the full chapter). |
Originally by traff on 2009-07-17 08:30:30 -0500 I have updated the ticket and the pdf. Reviewer's, please have a look |
Originally by traff on 2009-07-17 08:34:54 -0500 question on formatting: the line of second bullet-point is too long; where |
Originally by htor on 2009-07-17 13:20:20 -0500 Reviewed: OK |
Originally by jsquyres on 2009-07-20 13:07:16 -0500 PDF review: ok My $0.02: I'd put the line break before the entire term "edges[index[rank]-1]". |
Originally by RolfRabenseifner on 2009-07-21 06:33:35 -0500 PDF review: okay, but I would also prefer a line break before the first term "edges[index[rank-1]]". This linebreak should be done with |
Originally by traff on 2009-07-21 06:54:29 -0500 Agreed, and done |
Originally by traff on 2009-07-21 06:54:57 -0500 Attachment added: |
Originally by RolfRabenseifner on 2009-07-21 07:12:12 -0500 PDF-review and ticket-review (i.e., the modification since 2nd vote): both okay. |
Originally by gropp on 2009-07-22 11:08:25 -0500 WDG - Reviewed and complete. |
Originally by traff on 2009-08-30 06:05:50 -0500 As chapter editor, I took the liberty to set this to "complete" (ok, jeff?) |
Originally by jsquyres on 2010-09-18 04:56:21 -0500 This ticket is (long-since) complete; marking it closed. |
Originally by jsquyres on 2008-09-05 07:56:36 -0500
Description
Sep 8 2008 Straw Vote
|| Unique || Original || Abstain ||
|| 0 || 25 || 7 ||
MPI-2.1 specifically allows graph communicators to be created by listing a neighbor more than once in the node list. This results in an ambiguity in MPI_GRAPH_NEIGHBORS and MPI_GRAPH_NEIGHBORS_COUNT: should the list/count returned refer to a unique list of neighbors, or the original set of neighbors (including duplicates) that were used to create the communicator?
History
Ambiguity was accidentally introduced in MPI-2.1 (verified in July 2008 Forum meeting).
Proposed Solution
Per straw vote in the September 2008 Dublin Forum meeting, it was decided to return the non-unique list (i.e., the same values that were passed to the constructor). Specifically, remove the current description paragraph (from between MPI_GRAPH_NEIGHBORS_COUNT and MPI_GRAPH_NEIGHBORS) and add a new description paragraph (after MPI_GRAPH_NEIGHBORS) and add a new example explicitly showing the issue:
Remove "MPI_GRAPH_NEIGHBORS_COUNT and MPI_GRAPH_NEIGHBORS provide adjacency information for a general graph topology." from p251:34-35
Add the following after p252:5
MPI_GRAPH_NEIGHBORS_COUNT and MPI_GRAPH_NEIGHBORS provide adjacency information for a general graph topology. The returned count and array of neighbors for the queried rank will both include all neighbors and reflect the same edge ordering as was specified by the original call to MPI_GRAPH_CREATE. Specifically, MPI_GRAPH_NEIGHBORS_COUNT and MPI_GRAPH_NEIGHBORS will return values based on the original index and edges array passed to MPI_GRAPH_CREATE (assuming that index[-1] effectively equals zero):
• The count returned from MPI_GRAPH_NEIGHBORS_COUNT will be (index[rank] - index[rank-1]).
• The neighbors array returned from MPI_GRAPH_NEIGHBORS will be edges[index[rank-1]] through edges[index[rank]-1].
Add a new example showing the issue (see the tex below, or the attached PDF).
Here's the tex changes:
Impact on Implementations
Implementations will need to ensure that they save the extra information (i.e., nodes listed multiple times) so that they can be counted / returned later.
Impact on Applications / Users
Applications will need to ensure to handle the non-unique list that is returned.
Alternative Solutions
Forum decided that there shouldn't be an alternate solution -- all implementations should do the same thing.
Entry for the Change Log
Section 7.5.4 on page 248. Remove ambiguity regarding duplicated neighbors with MPI_GRAPH_NEIGHBORS and MPI_GRAPH_NEIGHBORS_COUNT.
The text was updated successfully, but these errors were encountered: