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

Fix Benchmark & Add Monopole Checks & Add Monopole Tests #497

Merged
merged 7 commits into from
Jul 2, 2024

Conversation

ChrisZYJ
Copy link
Contributor

@ChrisZYJ ChrisZYJ commented Jul 1, 2024

Description

  1. Fixes issue Delta GPUs results give NaNs in the viscous sub-grid bubble benchmark case whereas Phoenix GPUs do not #396 by adding missing parameter in viscous_weno5_sgb_mono case file.
  2. Add monopole checks and make some parameters compulsory
  3. New tests for monopole delay and all monopole supports (except for 6)

Explaination

The original benchmark viscous_weno5_sgb_mono did not specify the monopole support type, so it defaulted to 1. However, support = 1 is not valid for 3D simulations and triggered none of the if-statements in the f_delta function within the monopole module, resulting in f_delta being undefined. I'm not entirely sure why it only started failing on Frontier and Phoenix in my PR #495, but it's probably due to the floating-point numbers being checked differently.

The modified benchmark file should simulate what it intends to simulate, with the computational domain and parameters remaining the same. The only changes are adding Mono(1)%support' : 4 and a rotation of the domain to align the z-axis with the requirements of monopole support 4.

Now, I've made monopole support mandatory through checks (along with other monopole parameters) to prevent this from happening again. I've also added test cases for monopoles as requested (support 6 for cylindrical coordinates cannot be run, so no check is implemented).

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Scope

  • This PR comprises a set of related changes with a common goal

How Has This Been Tested?

Test Configuration:

  • What computers and compilers did you use to test this:
    • Ubuntu 22.04.4 LTS; GNU v11.4.0
    • Delta

Checklist

  • I have added comments for the new code
  • I have added regression tests to the test suite so that people can verify in the future that the feature is behaving as expected
  • I ran ./mfc.sh format before committing my code
  • New and existing tests pass locally with my changes, including with GPU capability enabled (both NVIDIA hardware with NVHPC compilers and AMD hardware with CRAY compilers) and disabled (not for AMD hardware)
  • This PR does not introduce any repeated code (it follows the DRY principle)
  • I cannot think of a way to condense this code and reduce any introduced additional line count

If your code changes any code source files (anything in src/simulation)

To make sure the code is performing as expected on GPU devices, I have:

  • Checked that the code compiles using NVHPC compilers
  • Ran the code on either V100, A100, or H100 GPUs and ensured the new feature performed as expected (the GPU results match the CPU results)

Performance tests are not done as the changes are strictly in the parameter checking and initialization stages, and only involves assigning values to variables.

@ChrisZYJ ChrisZYJ mentioned this pull request Jul 1, 2024
21 tasks
@sbryngelson
Copy link
Member

Good catch!

@sbryngelson sbryngelson merged commit 9630d35 into MFlowCode:master Jul 2, 2024
18 of 19 checks passed
AiredaleDev pushed a commit to AiredaleDev/MFC that referenced this pull request Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Delta GPUs results give NaNs in the viscous sub-grid bubble benchmark case whereas Phoenix GPUs do not
2 participants