Skip to content
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

conduit::blueprint::mpi::mesh::generate_index #618

Closed
nselliott opened this issue Sep 22, 2020 · 3 comments · Fixed by #621
Closed

conduit::blueprint::mpi::mesh::generate_index #618

nselliott opened this issue Sep 22, 2020 · 3 comments · Fixed by #621
Labels
Milestone

Comments

@nselliott
Copy link
Contributor

Just a comment/suggestion about this function in conduit::blueprint::mpi::mesh

void CONDUIT_BLUEPRINT_API generate_index(const conduit::Node &mesh,
                                          const std::string &ref_path,
                                          Node &index_out,
                                          MPI_Comm comm);

This is an MPI collective call, so all ranks must pass an index_out Node, but only one rank fills the Node with any data. Looks like it will be rank 0 as long as it has any domains on that rank, but in general you don't know. This means that the calling code needs to inspect index_out before doing anything with it subsequently, especially if it intends to pass it along to something that expects a non-empty Node.

I can deal with this by inspecting the DataType of the Node, and if it's an object, it must contain the generated index, but would it make sense to add something to to API that explicitly tells us which rank has the index in the Node? Some possibilities would be to make the rank the return value, or to add an Node& info to the signature as a place to put a small amount of metadata.

@cyrush
Copy link
Member

cyrush commented Sep 22, 2020

Thanks for the feedback -- I think I should simply fix to broadcast so that all ranks get the resulting index.
Seems like the easiest interface contract?

@nselliott
Copy link
Contributor Author

That's fine--I thought it may have been a choice to avoid the broadcast overhead. When I first looked at this, I assumed that the index would exist everywhere.

@cyrush
Copy link
Member

cyrush commented Sep 22, 2020

Great --- I'll go with that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants