Skip to content

Conversation

@jorgensd
Copy link
Member

@jorgensd jorgensd commented Sep 12, 2024

Resolve #3100.

The code gets a bit complex, so I will walk you through the idea here.

  1. All processes send indices that are ghosts on their process (and that they need in the sub-index map) to the owning process.
  2. On that process, we check if the process also needs this index. If it does, we:
    a. Assign ownership of this index to existing owner, which means that we know that all indices that sent this to the process, is in the dest_ranks of the sub-map.
  3. If the process does not need the index, we re-assign ownership to the lowest rank that requires this index.
    b. This means that we need to send all the other processes that require that index to the new owner, as they are new dest ranks.

The new code does this by using the (owner->ghost) communciator that already exists in the code, and sends only the unique set of new dest ranks to the new owner.
There we accumulate the dests from 2a. and 3b. into one unique sorted dest_rank list.

@jorgensd jorgensd marked this pull request as ready for review September 13, 2024 09:37
@garth-wells
Copy link
Member

@jpdean could you cast your eye over this?

@garth-wells garth-wells added this to the 0.9.0 milestone Sep 19, 2024
Copy link
Member

@jpdean jpdean left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, it looks good to me! Just a few comments. Also, maybe it would be clearer if things like new_owner were renamed to submap_owner etc?

[](auto& e) { return e.second; });
}
}
// Remove duplicate new dest ranks from recv process
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, can you explain why there would be duplicates? Since new_owner_dest_ranks is created from global_idx_to_possible_owner, do we not already have a unique set of ranks for each index?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only need the unique set of ranks for all newly assigned indices to the process.
I.e. if a process has taken over index i from process (N, M) and index j from (M, N, Q), what we want to send to this process is (M,N, Q), not (N,M,M,N,Q), as one would then have to do a sort and remove on all received indices.

Co-authored-by: Joe Dean <jpd62@cam.ac.uk>
@jorgensd
Copy link
Member Author

@jpdean what do you think about the revised comments? (in suggestions).

@garth-wells
Copy link
Member

Nice, it looks good to me! Just a few comments. Also, maybe it would be clearer if things like new_owner were renamed to submap_owner etc?

@jorgensd thoughts?

@jorgensd
Copy link
Member Author

Nice, it looks good to me! Just a few comments. Also, maybe it would be clearer if things like new_owner were renamed to submap_owner etc?

@jorgensd thoughts?

I made the name new_owner as that list only accounts for indices that have re-assigned ownership.
It does not account for dofs that: Already was owned by the processes, and was required by the owning process in the new map.

@garth-wells garth-wells added this pull request to the merge queue Sep 24, 2024
Merged via the queue into main with commit 0da58d1 Sep 24, 2024
@garth-wells garth-wells deleted the dokken/remove-nbx-submap branch September 24, 2024 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NBX should be removed from sub index map creation

4 participants