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

Use validation files for CI #371

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

Use validation files for CI #371

wants to merge 22 commits into from

Conversation

HomesGH
Copy link
Contributor

@HomesGH HomesGH commented Jan 6, 2025

Description

The way the CI for validation works is changed with this PR.
Formerly, the list of examples was run with the PR as well as the master and the result was compared. This could lead to some issues (see #319).
This is solved by comparing now to validation files that were generated in advance. This speeds up the CI and makes it easier to add items to the list of examples.

Additionally, the new solution checks if the results are consistent for various compilation options. This is done intrinsically since always the same validation files are used for comparison.

What is a good choice for the tolerance?

  • relative tolerance: 1e-8 as basic tolerance; adjusted to 5e-3 in two cases since the number of MPI ranks slightly influenced the results. In both cases, a generator instead of a checkpoint was used.

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

  • CI passed

Documentation

There are two new scripts (both in the checks folder):

  • validation_createJSON.py to generate the validation files based on the Logger output of ls1
  • validation_compare_files.py to compare the output of a simulation run with the validation file. This script is also used in the CI.

It is also possible to include the output of the ResultWriter provided that it gives parsable output (see #368).

@HomesGH HomesGH added the CI label Jan 6, 2025
@HomesGH HomesGH changed the title Use validation files during CI Use validation files for CI Jan 7, 2025
@HomesGH
Copy link
Contributor Author

HomesGH commented Jan 7, 2025

For now the CI fails due to very strict tolerance settings and the example ./DropletCoalescence/liq/config_1_generateLiq.xml. This might be fixed by resolving issue #372.

@HomesGH
Copy link
Contributor Author

HomesGH commented Jan 7, 2025

@cniethammer @FG-TUM This PR introduces a new way of validation by comparing the simulation output to a reference file. However, in one case of the examples list, the results of the simulation depend on the number of MPI ranks. Does this make sense to you or is it a bug / race condition / etc.?
This job succeeds while this one fails. All settings are the same besides the number of ranks. Currently, all jobs with Autopas=OFF (more examples in list) and 8 ranks fail. The rest succeeds.

@cniethammer
Copy link
Contributor

Just some short feedback having a first look:
I like the idea general idea of having meta information for validation purposes!
For more feedback, I will need some time to get through the details of the scripts/implementation.

@HomesGH
Copy link
Contributor Author

HomesGH commented Jan 30, 2025

It looks like the CI currently fails because of the extreme values in the beginning of the simulation of example DropletCoalescence/liq/config_1_generateLiq.xml. The extreme values are due to the GridFiller, see PR #375 and issues mentioned there.
A workaround could be to use the CubicGridGenerator instead, but there is another issue #376 when using particles with a mass other than 1.

@HomesGH
Copy link
Contributor Author

HomesGH commented Jan 30, 2025

It looks like the CI currently fails because of the extreme values in the beginning of the simulation of example DropletCoalescence/liq/config_1_generateLiq.xml. The extreme values are due to the GridFiller, see PR #375 and issues mentioned there. A workaround could be to use the CubicGridGenerator instead, but there is another issue #376 when using particles with a mass other than 1.

Issue #376 is fixed in PR #377. After merging PR #377, we could then switch the failing example to use the CubicGridGenerator which would probably prevent the present CI from failure.

@HomesGH HomesGH marked this pull request as ready for review February 6, 2025 09:42
@HomesGH HomesGH requested a review from cniethammer February 6, 2025 09:42
@cniethammer
Copy link
Contributor

I will not be able to review this before next week - Just to let know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants