Test all setups with MPI in the workflow #217
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
the matrix variable was not being passed through to make
using only 2 threads may not catch all bugs
4 tasks ensures most degeneracies are lifted 1 task tests the trivial case (which is special for tree building)
so 4 tasks can run on runners with 2 cores
|
We are failing the sink particle creation test. This attempts to create a sink particle at the location of the SPH particle with maximum density (rhomax). So something must be missing with the reduction across MPI threads for this. |
#234 should fix this 🙂 |
conradtchan
added a commit
that referenced
this pull request
Feb 17, 2022
Type of PR: Bug fix Description: Required for #217, discovered when `mpirun -n 4` was added In ptmass.F90, during sink creation, whether the particle is at the potential minimum is checked: ``` ! CHECK 7: particle i is at minimum in potential if (poteni > potenj_min) then ifail = inosink_poten ifail_array(inosink_poten) = 1 endif ``` This check fails because an identical particle distribution gets created on every task, resulting in close comparisons when determining which particle to create a sink at. This is fixed by only creating the particles on the master task, and allowing derivs to rebalance them. `itest` needs to be calculated after calling derivs or else it will be incorrect. The `xcofm` calculation has been removed because the result is not used anywhere. Testing: ``` make MPI=yes SETUP=testgrav phantomtest mpirun -n 4 bin/phantomtest ptmass ``` Did you run the bots? yes, pre-commit
conradtchan
added a commit
that referenced
this pull request
Feb 17, 2022
Type of PR: Bug fix Description: Required for #217 In a93bc90, as part of implementing MPI support for nimhd tests, the L2 error variables were reduced across tasks. There is a typo resulting in the `v_x` and `B_y` error being overwritten by the density error. This produces a test failure due to the tolerance not being met. In df72df1, NICIL 2.1 was added to phantom, where the tolerance for `B_y` was changed to `tolb=3.1d-2`. This was change was made possibly because the apparent improved accuracy was wrongly attributed to improvements to the code, rather than because the the density error was overwriting the `B_y` error. `tolb` has been reverted to the original value of `1.1d-1`. Testing: ``` make MPI=yes SETUP=testnimhd OPENMP=no phantomtest mpirun -n 4 bin/phantomtest nimhd ``` Did you run the bots? no, not required
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Type of PR:
other
Description:
Currently, only the
testkdsetup is tested against MPI. There is also a bug in the action that causesDEBUG=yesto not be tested. This PR adds every test from the serial tests.Resolves #210
Resolves #231 by testing:
Testing:
Not all tests currently pass. Tests should be fixed before merging this PR.
Did you run the bots? yes