Update restart files of application tests for p4est 2.3.6 #1181

acdaigneault · 2024-06-28T00:17:41Z

Description

deal.II changed the version of the dependancy p4est to 2.3.6 for their docker master-focal (the one we use for our CI with deal.II version master). We were using p4est 2.3.2, which makes some of our application-test restart files not compatible with the new p4est version.
Unfortunately, the previous restart files were generated a long time ago (> 9 months). The .prm has not been updated since then and they were a bit all over the place.

Solution

Update the .prm files with the proper parameter names.
Update the restart files that were failling.
Gather the .prm for the lethe-fluid-particles and lethe-fluid-vans .prm generator in the generator folder.
If the .prm restart generator file was not there, new ones have been created trying to replicated it.

Testing

All tests pass on CI with deal.II version master and they are now disabled with deal.II version 9.5.0.

Documentation

The next PR will modify the installation doc according to p4est 2.3.6

Miscellaneous (will be removed when merged)

Important note: The adaptive sparse contact test with lethe-fluid-particles is now a dummy test since it does not test all the mobility status anymore. I can say if it is a bug, if the .prm files to generated the restart files were wrong or if the small modification in DEM change the status in cfd-dem. I will open an issue about it since the mechanism is not properly tested at the moment see #1182

I kept all the generated restart files (even .pvdhandler) in case someone wants output from restarts

Checklist (will be removed when merged)

See this page for more information about the pull request process.

Code related list:

All in-code documentation related to this PR is up to date (Doxygen format)
Lethe documentation is up to date
Fix has unit test(s) (preferred) or application test(s)
The branch is rebased onto master
Changelog (CHANGELOG.md) is up to date
Code is indented with indent-all and .prm files (examples and tests) with prm-indent

Pull request related list:

Labels are applied
There are at least 2 reviewers (or 1 if small feature) excluding the responsible for the merge
If this PR closes an issue or is related to a project, it is linked in the "Projects" or "Development" section
If the fix is temporary, an issue is opened
The PR description is cleaned and ready for merge

Clean up in .prm files All has been moved to folder generator and path to the restart folder has been add in the .prm Some .prm are missing Tests has changed, the restarts files were pretty old >9 months, so hard to say if this is an issue or just the DEM has changed to much

Remove the redundant restart files (they were all the same, they cmake just need to copy the same)

OGaboriault

Tiny comment. I think I will work on a script to automatically generate the restart files for every application tests we have.

CHANGELOG.md

acdaigneault · 2024-06-29T00:20:39Z

@OGaboriault cool! Also, I did not redo all the restart files, only the ones that were failling and a few more. If we want fully automated generation of everything, we have to regenerate all restart and have a standardization to target them.
I was also thinking about having the restart files generator included in the check parameter script for examples. But the CI job for that one would need a new name

OresteMarquis

Not much to say, If all the test work its good for me :) It seems like a lot of reorganization, I like it! Congrats Audrey, It was probably a pain to update everything, thanks for your work :)

OresteMarquis · 2024-06-29T18:59:39Z

.github/workflows/main_release.yml

@@ -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 }}
-
+


I would add an error when matrix.dealii_version != 'master' for clarity.
Something like:

Suggested change

if: ${{ matrix.dealii_version == 'master'}}

run: ...

else:

error:

message: "Version of deal.ii for tests is not the same as their master image."

code: 1

details: "Restart files are not compatible with deal.ii v9.5.0"

That's a cool suggestion.... as long as that does not throw an error I am down for that.

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?

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

So I don't change anything at the moment?

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

OresteMarquis · 2024-06-29T19:00:51Z

.github/workflows/main_release.yml

@@ -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 }}


I would add the same else error as the previous comment here.

applications_tests/lethe-fluid-particles/CMakeLists.txt

OresteMarquis · 2024-06-29T19:09:46Z

applications_tests/lethe-fluid-particles/adaptive_sparse_contacts.mpirun=1.output

Does the new p4est version cause the changes in the application_tests? Or did the tests have been changed change?

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.
I build this test and it was pretty hard to get all the status with a very small number of particles. The test is probably very sensitive to the initial condition. Or the generation files in the repo were not the good ones.

OresteMarquis · 2024-06-29T19:14:43Z

...ations_tests/lethe-fluid-particles/generators/adaptative_sparse_contacts_generator_step1.prm

I'm just curious, I don't even know what it does, but why does the output frequency has been set to 0?

It means that we do not generate output files. It's not a good idea to generate output files in the tests since i tends to slow down the CI significantly.

Actually, in fluid it does otuput the one at time = 0. Would it be better to write -1 for all tests? In DEM writing 0 does not generate outputs

Let's maybe do that in another PR where we just do that. For the time being let's keep this PR at the curent level it is.

OresteMarquis · 2024-06-29T19:44:40Z

applications_tests/lethe-fluid/CMakeLists.txt

If I understand correctly, we don't test .vtu anymore?

I don't think this test was testing that, especially since the test to not write any .vtu. But this is a very good point since no other test seems to test that for lethe-fluid!
And yeah, I just saw the person that did that was Audrey in 2020, and I guarantee that I did not really know what I was doing back then haha
Do you think it's a good idea to add them back and add outputs to the test?

applications_tests/lethe-fluid-vans/dallavalle_packed_bed_files/dem.particles

blaisb

I'll merge directly after you have addressed @OresteMarquis last suggestion. Very good work. Thank you so much for having taken care of this.

blaisb · 2024-06-29T21:57:26Z

.github/workflows/main_release.yml

@@ -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 }}
-
+


That's a cool suggestion.... as long as that does not throw an error I am down for that.

applications_tests/lethe-fluid-particles/restart_fluidized_bed_files/dem.triangulation.info

blaisb · 2024-06-30T06:21:51Z

@acdaigneault Good to merge on my side, I'll wait for your final GO and then I'll merge

Description This PR follows #1181 and updates documentation related to the installation of Lethe. More specifically, it corrects the version of p4est to be installed to 2.3.6. Co-authored-by: Olivier Guévremont <[email protected]>

Description The ASC applications was not testing all the mobility status after the change of restart files in PR #1181. Solution Change the granular temperature threshold to get the 3 mobility status at the start of the simulation and output the status sooner. Close issue #1182.

…mtl#1181) Description deal.II changed the version of the dependancy p4est to 2.3.6 for their docker master-focal (the one we use for our CI with deal.II version master). We were using p4est 2.3.2, which makes some of our application-test restart files not compatible with the new p4est version. Unfortunately, the previous restart files were generated a long time ago (> 9 months). The .prm has not been updated since then and they were a bit all over the place. Solution Update the .prm files with the proper parameter names. Update the restart files that were failling. Gather the .prm for the lethe-fluid-particles and lethe-fluid-vans .prm generator in the generator folder. If the .prm restart generator file was not there, new ones have been created trying to replicated it. Testing All tests pass on CI with deal.II version master and they are now disabled with deal.II version 9.5.0. Documentation The next PR will modify the installation doc according to p4est 2.3.6 Former-commit-id: 4dd53b3

Description This PR follows chaos-polymtl#1181 and updates documentation related to the installation of Lethe. More specifically, it corrects the version of p4est to be installed to 2.3.6. Co-authored-by: Olivier Guévremont <[email protected]> Former-commit-id: 7405173

…ymtl#1201) Description The ASC applications was not testing all the mobility status after the change of restart files in PR chaos-polymtl#1181. Solution Change the granular temperature threshold to get the 3 mobility status at the start of the simulation and output the status sooner. Close issue chaos-polymtl#1182. Former-commit-id: 8ae0d4d

Description deal.II changed the version of the dependancy p4est to 2.3.6 for their docker master-focal (the one we use for our CI with deal.II version master). We were using p4est 2.3.2, which makes some of our application-test restart files not compatible with the new p4est version. Unfortunately, the previous restart files were generated a long time ago (> 9 months). The .prm has not been updated since then and they were a bit all over the place. Solution Update the .prm files with the proper parameter names. Update the restart files that were failling. Gather the .prm for the lethe-fluid-particles and lethe-fluid-vans .prm generator in the generator folder. If the .prm restart generator file was not there, new ones have been created trying to replicated it. Testing All tests pass on CI with deal.II version master and they are now disabled with deal.II version 9.5.0. Documentation The next PR will modify the installation doc according to p4est 2.3.6 Former-commit-id: 4dd53b3

Description This PR follows #1181 and updates documentation related to the installation of Lethe. More specifically, it corrects the version of p4est to be installed to 2.3.6. Co-authored-by: Olivier Guévremont <[email protected]> Former-commit-id: 7405173

Description The ASC applications was not testing all the mobility status after the change of restart files in PR #1181. Solution Change the granular temperature threshold to get the 3 mobility status at the start of the simulation and output the status sooner. Close issue #1182. Former-commit-id: 8ae0d4d

acdaigneault added Bug Something isn't working Maintenance WIP When a PR is open but not ready for review labels Jun 28, 2024

acdaigneault force-pushed the update_restart_files branch from 7a9692b to 7e6f707 Compare June 28, 2024 14:39

acdaigneault added 5 commits June 28, 2024 10:41

Remove useless dem restart in cmake file

fccba1d

Update the lethe-fluid-vans restart files

fc0755b

Remove the redundant restart files (they were all the same, they cmake just need to copy the same)

Update poiseuille restart files

cca24f4

Add a restart file generator for sharp

5fff0c3

Update lethe-particles tests

256a151

acdaigneault requested review from OGaboriault, blaisb and OresteMarquis June 28, 2024 21:37

acdaigneault added 2 commits June 28, 2024 17:38

Change log

69c2c16

Indent

1b4a5d0

acdaigneault requested review from hepap and removed request for OGaboriault June 28, 2024 21:44

acdaigneault mentioned this pull request Jun 28, 2024

The adaptive_sparse_contacts application test with lethe-fluid-particles is wrong #1182

Closed

acdaigneault changed the title ~~Update restart files of application test for p4est 2.3.6 [wip]~~ Update restart files of application test for p4est 2.3.6 Jun 28, 2024

acdaigneault added Ready for review and removed WIP When a PR is open but not ready for review labels Jun 28, 2024

OGaboriault approved these changes Jun 28, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

acdaigneault added 3 commits June 29, 2024 11:19

Attempt to unable test with deal.ii v9.5

7f7c874

Fix typo

6a58c6c

Use yaml ifs

739152b

acdaigneault added CI and removed Bug Something isn't working labels Jun 29, 2024

acdaigneault changed the title ~~Update restart files of application test for p4est 2.3.6~~ Update restart files of application tests for p4est 2.3.6 Jun 29, 2024

OresteMarquis approved these changes Jun 29, 2024

View reviewed changes

AmishgaAlphonius mentioned this pull request Jun 29, 2024

Update installation doc #1183

Merged

9 tasks

blaisb approved these changes Jun 29, 2024

View reviewed changes

acdaigneault added 2 commits June 30, 2024 18:03

Change dp in dem generator of restart fluidized bed file

502c4b8

Change position of initial particles in some test

3660833

acdaigneault added Reviewed and ready to merge and removed Ready for review labels Jun 30, 2024

Remove the statistics

fdfc87f

blaisb merged commit 4dd53b3 into master Jul 1, 2024
8 checks passed

blaisb deleted the update_restart_files branch July 1, 2024 07:42

acdaigneault mentioned this pull request Jul 19, 2024

Fix the ASC application test for CFD-DEM #1201

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update restart files of application tests for p4est 2.3.6 #1181

Update restart files of application tests for p4est 2.3.6 #1181

acdaigneault commented Jun 28, 2024 •

edited

Loading

OGaboriault left a comment

acdaigneault commented Jun 29, 2024

OresteMarquis left a comment

OresteMarquis Jun 29, 2024

blaisb Jun 29, 2024

acdaigneault Jun 29, 2024

blaisb Jun 30, 2024

acdaigneault Jun 30, 2024

blaisb Jun 30, 2024

OresteMarquis Jun 29, 2024

OresteMarquis Jun 29, 2024

acdaigneault Jun 29, 2024

OresteMarquis Jun 29, 2024

blaisb Jun 29, 2024

acdaigneault Jun 30, 2024

blaisb Jun 30, 2024

OresteMarquis Jun 29, 2024

acdaigneault Jun 30, 2024

blaisb left a comment

blaisb Jun 29, 2024

blaisb commented Jun 30, 2024

+if: ${{ matrix.dealii_version == 'master'}}
+     run: ...
+else:
+     error:
+         message: "Version of deal.ii for tests is not the same as their master image."
+      code: 1
+         details: "Restart files are not compatible with deal.ii v9.5.0"

Update restart files of application tests for p4est 2.3.6 #1181

Update restart files of application tests for p4est 2.3.6 #1181

Conversation

acdaigneault commented Jun 28, 2024 • edited Loading

Description

Solution

Testing

Documentation

Miscellaneous (will be removed when merged)

Checklist (will be removed when merged)

OGaboriault left a comment

Choose a reason for hiding this comment

acdaigneault commented Jun 29, 2024

OresteMarquis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blaisb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blaisb commented Jun 30, 2024

acdaigneault commented Jun 28, 2024 •

edited

Loading