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

Allow CubeGenerator to use multiple MPI processes #1105

Merged
merged 24 commits into from
May 22, 2024

Conversation

montrie
Copy link
Contributor

@montrie montrie commented May 7, 2024

Extended the CubeGenerator class to allow for using more than 1 MPI process.

Boundary condition 7 works as expected, however using boundary condition 6 leads to exponential growth in the elastic energy output when using more than 1 MPI process. This also happens with the NetcdfReader and meshes generated with the cubeGenerator located in preprocessing/meshing/cube_c/. I added a warning to the CubeGenerator class which warns the user of this issue when using at least 2 MPI processes and boundary condition 6.

@davschneller
Copy link
Contributor

Hi, thanks for the PR! Would it be possible to upgrade to the current master branch? There may still be some file conflicts/file moves there.

@montrie
Copy link
Contributor Author

montrie commented May 8, 2024

I merged the upstream changes into the current master branch. After fixing the merge-related issues, I ran into the following error when I tried to compile SeisSol:

[ 97%] Building CXX object CMakeFiles/SeisSol-bin.dir/src/main.cpp.o
/dss/dsshome1/lxc0B/ga27koz2/apps/fork_SeisSol/src/main.cpp(100): error: namespace "MPI" has no member "mpi"
    MPI::mpi.init(argc, argv);
         ^

/dss/dsshome1/lxc0B/ga27koz2/apps/fork_SeisSol/src/main.cpp(101): error: namespace "MPI" has no member "mpi"
    const int rank = MPI::mpi.rank();
                          ^

/dss/dsshome1/lxc0B/ga27koz2/apps/fork_SeisSol/src/main.cpp(132): error: namespace "MPI" has no member "mpi"
      MPI::mpi.finalize();
           ^

compilation aborted for /dss/dsshome1/lxc0B/ga27koz2/apps/fork_SeisSol/src/main.cpp (code 2)
make[2]: *** [CMakeFiles/SeisSol-bin.dir/build.make:76: CMakeFiles/SeisSol-bin.dir/src/main.cpp.o] Error 2
make[1]: *** [CMakeFiles/Makefile2:146: CMakeFiles/SeisSol-bin.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
[ 98%] Linking CXX static library libSeisSol-proxy-core.a
[ 98%] Built target SeisSol-proxy-core
make: *** [Makefile:136: all] Error 2

I fixed it locally by adding the seissol:: namespace to the calls to the MPI class above. Should I include these changes in this PR as well, or is this just an issue on my end?

@davschneller
Copy link
Contributor

davschneller commented May 15, 2024

I merged the upstream changes into the current master branch. After fixing the merge-related issues, I ran into the following error when I tried to compile SeisSol:

[ 97%] Building CXX object CMakeFiles/SeisSol-bin.dir/src/main.cpp.o
/dss/dsshome1/lxc0B/ga27koz2/apps/fork_SeisSol/src/main.cpp(100): error: namespace "MPI" has no member "mpi"
    MPI::mpi.init(argc, argv);
         ^

/dss/dsshome1/lxc0B/ga27koz2/apps/fork_SeisSol/src/main.cpp(101): error: namespace "MPI" has no member "mpi"
    const int rank = MPI::mpi.rank();
                          ^

/dss/dsshome1/lxc0B/ga27koz2/apps/fork_SeisSol/src/main.cpp(132): error: namespace "MPI" has no member "mpi"
      MPI::mpi.finalize();
           ^

compilation aborted for /dss/dsshome1/lxc0B/ga27koz2/apps/fork_SeisSol/src/main.cpp (code 2)
make[2]: *** [CMakeFiles/SeisSol-bin.dir/build.make:76: CMakeFiles/SeisSol-bin.dir/src/main.cpp.o] Error 2
make[1]: *** [CMakeFiles/Makefile2:146: CMakeFiles/SeisSol-bin.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
[ 98%] Linking CXX static library libSeisSol-proxy-core.a
[ 98%] Built target SeisSol-proxy-core
make: *** [Makefile:136: all] Error 2

I fixed it locally by adding the seissol:: namespace to the calls to the MPI class above. Should I include these changes in this PR as well, or is this just an issue on my end?

Hi, I'm gonna embed the namespace fixes also into #1112—if that's good for you. (it appeared now in other contexts as well, cf. #1113 ; I'll probably just namespace the main SeisSol class)

Copy link
Contributor

@davschneller davschneller left a comment

Choose a reason for hiding this comment

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

Hi, thank you for your PR.
I've added some minor comments regarding the code.
Fixing boundary condition 6 may be a bit more involved in the end—so it's not required for this PR.

src/Solver/time_stepping/TimeManager.cpp Outdated Show resolved Hide resolved
src/Solver/Simulator.cpp Outdated Show resolved Hide resolved
src/Initializer/time_stepping/MultiRate.hpp Outdated Show resolved Hide resolved
src/Initializer/time_stepping/MultiRate.hpp Outdated Show resolved Hide resolved
src/Initializer/ParameterDB.cpp Outdated Show resolved Hide resolved
src/Geometry/CubeGenerator.cpp Outdated Show resolved Hide resolved
src/Geometry/CubeGenerator.cpp Show resolved Hide resolved
src/Geometry/CubeGenerator.cpp Outdated Show resolved Hide resolved
src/Geometry/CubeGenerator.cpp Outdated Show resolved Hide resolved
src/Geometry/CubeGenerator.cpp Outdated Show resolved Hide resolved
@montrie
Copy link
Contributor Author

montrie commented May 21, 2024

Thank you for the suggestions, let me know if I missed something!

Copy link
Contributor

@davschneller davschneller left a comment

Choose a reason for hiding this comment

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

Ok, thanks for the changes! I've mirrored the branch to run it through the tests (we currently need to do that for external branches). And it passed.

@davschneller davschneller added this pull request to the merge queue May 22, 2024
Merged via the queue into SeisSol:master with commit 5ecc5ff May 22, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants