search

trilinos / Trilinos

Primary repository for the Trilinos Project
https://trilinos.org/
Other
1.21k stars 567 forks source link

Teuchos::Comm: Add cheap conversion between Comm<U> and Comm<V> #5455

Open mhoemmen opened 5 years ago

mhoemmen commented 5 years ago

Bug Report

@trilinos/teuchos @cgcgcg @trilinos/thyra @trilinos/tpetra

Description

It should be easy and cheap to convert between Teuchos::Comm<int> and Teuchos::Comm<T> for any integer T. We need this for interactions between Tpetra and Thyra, e.g., for PR https://github.com/trilinos/Trilinos/pull/5400.

csiefer2 commented 5 years ago

We should have just deprecated that template parameter...

kddevin commented 5 years ago

I'm curious: why does code exist using Comm with V != int? Teuchos::Comm with MPI works only with int, right?
Deprecating is a good idea. Finding someone with cycles to do it is the problem. :(

ibaned commented 5 years ago

I agree that we should basically static_assert(std::is_same<T, int>::value, "anything else is crazy") and eventually remove the template parameter. MPI uses int, and no alternative is anywhere on our radar. Top500 supercomputers have plateaued around supporting a maximum of one million MPI ranks and don't show many signs of approaching 2 billion MPI ranks.

bartlettroscoe commented 5 years ago

I int not also used for the size of the buffers of data being copied? With an signed 32 bit integer, that tops off at just over 2 billion elements, no? Is that not still an issue?

But I agree that Teuchos::Comm<> should not have been templated (I argued against it at the time).

mhoemmen commented 5 years ago

The MPI Standard is moving towards separating MPI process rank from counts and displacements. Process ranks will stay int. See e.g., https://github.com/mpi-forum/mpi-issues/issues/137 .

ibaned commented 5 years ago

We could just change the methods of Teuchos::Comm to accept a signed 64-bit integer as their count argument, and convert to whatever the underlying MPI supports as necessary internally.

mhoemmen commented 5 years ago

Jeff Hammond's "BigMPI" paper and library shows a library-based solution. Process ranks remain 32 bits.

mhoemmen commented 5 years ago

@ibaned wrote:

We could just change the methods of Teuchos::Comm to accept a signed 64-bit integer as their count argument, and convert to whatever the underlying MPI supports as necessary internally.

This would be an interesting way to maintain backwards compatibility with older MPI versions, once MPI BigCount lands.

mhoemmen commented 5 years ago

@bartlettroscoe wrote:

But I agree that Teuchos::Comm<> should not have been templated (I argued against it at the time).

Thank you for fighting for sanity :-) . People forget that Comm, BLAS, and LAPACK started in the Tpetra namespace, circa 2004.

github-actions[bot] commented 3 years ago

This issue has had no activity for 365 days and is marked for closure. It will be closed after an additional 30 days of inactivity. If you would like to keep this issue open please add a comment and/or remove the MARKED_FOR_CLOSURE label. If this issue should be kept open even with no activity beyond the time limits you can add the label DO_NOT_AUTOCLOSE. If it is ok for this issue to be closed, feel free to go ahead and close it. Please do not add any comments or change any labels or otherwise touch this issue unless your intention is to reset the inactivity counter for an additional year.

mhoemmen commented 3 years ago

begone autocloser

github-actions[bot] commented 2 years ago

This issue has had no activity for 365 days and is marked for closure. It will be closed after an additional 30 days of inactivity. If you would like to keep this issue open please add a comment and/or remove the MARKED_FOR_CLOSURE label. If this issue should be kept open even with no activity beyond the time limits you can add the label DO_NOT_AUTOCLOSE. If it is ok for this issue to be closed, feel free to go ahead and close it. Please do not add any comments or change any labels or otherwise touch this issue unless your intention is to reset the inactivity counter for an additional year.