-
Notifications
You must be signed in to change notification settings - Fork 61
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
Update restart files of application tests for p4est 2.3.6 #1181
Changes from 11 commits
7e6f707
fccba1d
fc0755b
cca24f4
5fff0c3
256a151
69c2c16
1b4a5d0
7f7c874
6a58c6c
739152b
502c4b8
3660833
fdfc87f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -63,7 +63,10 @@ jobs: | |
make -j${{ env.COMPILE_JOBS }} | ||
# These tests require a single core each so we will run them in parallel | ||
# Only run tests on deal.ii master version | ||
# Restart files are not compatible with deal.ii v9.5.0 | ||
- name: Run Lethe tests (Release-deal.ii:${{ matrix.dealii_version }}) | ||
if: ${{ matrix.dealii_version == 'master'}} | ||
run: | | ||
#Allow OMPI to run as root | ||
export OMPI_ALLOW_RUN_AS_ROOT=1 | ||
|
@@ -73,9 +76,13 @@ jobs: | |
ctest -N --exclude-regex ${{ env.MULTI_CORE_TESTS_REGEX }} | ||
# Run in parallel | ||
ctest --output-on-failure -j${{ env.COMPILE_JOBS }} --exclude-regex ${{ env.MULTI_CORE_TESTS_REGEX }} | ||
# These tests require two cores each so we will run them sequencially | ||
# Only run tests on deal.ii master version | ||
# Restart files are not compatible with deal.ii v9.5.0 | ||
- name: Run multi-core Lethe tests (Release-deal.ii:${{ matrix.dealii_version }}) | ||
if: ${{ matrix.dealii_version == 'master'}} | ||
run: | | ||
#Allow OMPI to run as root | ||
export OMPI_ALLOW_RUN_AS_ROOT=1 | ||
|
@@ -84,4 +91,4 @@ jobs: | |
# Print the tests to be executed | ||
ctest -N --tests-regex ${{ env.MULTI_CORE_TESTS_REGEX }} | ||
# Run sequencially | ||
ctest --output-on-failure --tests-regex ${{ env.MULTI_CORE_TESTS_REGEX }} | ||
ctest --output-on-failure --tests-regex ${{ env.MULTI_CORE_TESTS_REGEX }} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would add the same else error as the previous comment here. |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does the new p4est version cause the changes in the application_tests? Or did the tests have been changed change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this case I'm not sure, but I don't think so. A few things have changed in the DEM solver that causes tiny differences. The mobility status can change in one iteration. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
0 0 350 74 350 | ||
0 0 350 78 350 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,2 @@ | ||
0 | ||
Time File | ||
Time File |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add an error when matrix.dealii_version != 'master' for clarity.
Something like:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a cool suggestion.... as long as that does not throw an error I am down for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a documentation of the else statement and error? I don't find it in the GitHub action documentation.
Also, giving an error won't failed the check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it gives an error it would indeed fail the check.
To keep the PR finalized because it's a critical one, let's postpone this check to a next PR. We can open an issue ot keep track of this idea, but to be honest, right now just writing a comment is all good for the time being. Throwing an error would break the CI and so code: 1 would most likely give an error.
Let's postpone that for the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I don't change anything at the moment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, let's not change anything. Let's merge the PR as-is when you are ready with the current feature set. Then if we want to change other stuff, let's make small PRs for them