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

Stratimikos: use MPI in Amesos adapter test #1083

Closed

Conversation

ibaned
Copy link
Contributor

@ibaned ibaned commented Feb 22, 2017

The Amesos interface to SuperLU_DIST needs to
give SuperLU_DIST and MPI_Comm, therefore it
needs to be given an Epetra_MpiComm, even on
a single rank.
Without this commit, Stratimikos+Amesos+SuperLU_DIST
testing will always fail with a bad_cast
from Epetra_SerialComm to Epetra_MpiComm.

This is leading up to more work I'm doing adding
Amesos2 to Stratimikos, will show up in a future PR.

@trilinos/stratimikos

The Amesos interface to SuperLU_DIST needs to
give SuperLU_DIST and MPI_Comm, therefore it
needs to be given an Epetra_MpiComm, even on
a single rank.
Without this commit, Stratimikos+Amesos+SuperLU_DIST
testing will always fail with a bad_cast
from Epetra_SerialComm to Epetra_MpiComm.
@ibaned ibaned added type: bug The primary issue is a bug in Trilinos code or tests pkg: Stratimikos labels Feb 22, 2017
@ibaned ibaned added the stage: in progress Work on the issue has started label Feb 22, 2017
@bartlettroscoe
Copy link
Member

@ibaned, looks pretty straightforward. I guess since we have no automated tests for Trilinos that include SuperLU_Dist, that is why we are never seeing any failing tests?

Anyway, this looks fine to push to me. Do you want me to pull, rebase, and push this commit to Trilinos? This will just ensure that existing tests are passing. I can't test with SuperLU_Dist since I don't have a machine with a standard build of this TPL.

@mhoemmen
Copy link
Contributor

Should this be MPI_COMM_SELF instead of MPI_COMM_WORLD? Just curious :)

@ibaned
Copy link
Contributor Author

ibaned commented Feb 22, 2017

@bartlettroscoe: Yes, I assume there must not be automated testing with SuperLU_Dist, I don't think it can work before this commit.

If you can check existing tests and push, that would be appreciated. Conversely, you prefer to save time by letting me push fairly small changes to Stratimikos directly, I can start doing that.

I have done the testing with SuperLU_Dist enabled, and the Stratimikos-Amesos test passes with this commit.

@mhoemmen: the way this test is set up it pretty clearly always runs on one rank, so I thought it shouldn't make a difference. If it is ever made to run in parallel, I assume that would be to test distributed solves, so I chose COMM_WORLD as one less thing that would have to be changed if the test is ever expanded to run with multiple ranks.

@mhoemmen
Copy link
Contributor

@ibaned cool just curious :)

@bartlettroscoe
Copy link
Member

If you can check existing tests and push, that would be appreciated. Conversely, you prefer to save time by letting me push fairly small changes to Stratimikos directly, I can start doing that.

I have done the testing with SuperLU_Dist enabled, and the Stratimikos-Amesos test passes with this commit.

@ibaned, it is fine if you push. But it is not bad to have some review like this first. But when you push, please use the standard CI build with checkin-test-sems.sh on a machine that mounts the SEMS ENV. Since you are only changing the Stratimikos tests, you can safely run with:

./checkin-test-sems.sh --no-enable-fwd-packages --do-all --push

Let me know if you have any questions about that.

@ibaned
Copy link
Contributor Author

ibaned commented Feb 22, 2017

Okay, I managed to use the checkin script with SEMS to push 5e89250. In the future, I'll still open PRs for review and once the review is satisfactory I may do the checkin push myself, depending on the nature of the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: Stratimikos type: bug The primary issue is a bug in Trilinos code or tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants