GRCBC #698

anandrdbz · 2024-11-06T22:14:29Z

Description

Generalized CBC implementation given by Pirozolli and Colonius (2013). Provides the option to supplement non-reflecting subsonic inflow and subsonic outflow boundary conditions (bc = -7 and -8) with additional terms. Their effect can be turned on by setting bc%grcbc_in and bc%grcbc_out to be true (false by default). Additional option to turn on bc%grcbc_vel_out that includes outflow velocity specification.

If turned on, inflow boundary conditions require inflow velocities, pressure ,density and volume fraction specifications outside the domain. For outflow conditions, pressure and normal velocity specifications needed, although the normal velocity can be turned off using bc%grcbc_vel_out.

Refer to case file 2D_ibm_steady_shock for an example with GRCBC inflow conditions for a shock with a given velocity.

2 additional cases added to the examples directory.

2D_zero_circ_vortex advects a zero circulation isentropic vortex in the middle of the domain from left to right with Subsonic Inflow and subsonic outflow boundary conditions.

2D_acoustic_pulse has a radially outward pressure pulse with subsonic outflow boundary conditions on all 4 sides.

Fixes #(issue) [optional]

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
[x ] New feature (non-breaking change which adds functionality)
Something else

Scope

[ x] This PR comprises a set of related changes with a common goal

If you cannot check the above box, please split your PR into multiple PRs that each have a common goal.

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.
Provide instructions so we can reproduce.
Please also list any relevant details for your test configuration

Test A
Test B

Test Configuration:

What computers and compilers did you use to test this:

Checklist

I have added comments for the new code
I added Doxygen docstrings to the new code
I have made corresponding changes to the documentation (docs/)
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 have added example cases in examples/ that demonstrate my new feature performing as expected.
They run to completion and demonstrate "interesting physics"
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
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
Checked that the code compiles using CRAY 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)
Ran the code on MI200+ GPUs and ensure the new features performed as expected (the GPU results match the CPU results)
Enclosed the new feature via nvtx ranges so that they can be identified in profiles
Ran a Nsight Systems profile using ./mfc.sh run XXXX --gpu -t simulation --nsys, and have attached the output file (.nsys-rep) and plain text results to this PR
Ran an Omniperf profile using ./mfc.sh run XXXX --gpu -t simulation --omniperf, and have attached the output file and plain text results to this PR.
Ran my code using various numbers of different GPUs (1, 2, and 8, for example) in parallel and made sure that the results scale similarly to what happens if you run without the new code/feature

codecov · 2024-11-08T02:43:02Z

Codecov Report

Attention: Patch coverage is 44.44444% with 25 lines in your changes missing coverage. Please review.

Project coverage is 42.98%. Comparing base (9c2d3d9) to head (3792b88).
Report is 5 commits behind head on master.

Files with missing lines	Patch %	Lines
src/simulation/m_cbc.fpp	28.57%	17 Missing and 8 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #698      +/-   ##
==========================================
+ Coverage   42.96%   42.98%   +0.01%     
==========================================
  Files          61       61              
  Lines       16339    16384      +45     
  Branches     1881     1889       +8     
==========================================
+ Hits         7020     7042      +22     
- Misses       8278     8293      +15     
- Partials     1041     1049       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sbryngelson · 2024-11-08T21:01:03Z

Please fix merge conflicts

anandrdbz · 2024-11-11T19:43:43Z

@sbryngelson the checks passed after conflict resolution, so should be ready for merge

sbryngelson · 2024-11-11T20:25:00Z

thanks @anandrdbz I will review this soon!

examples/2D_acoustic_pulse/case.py

examples/2D_ibm_steady_shock/case.py

examples/2D_zero_circ_vortex/case.py

sbryngelson · 2024-11-12T00:44:00Z

docs/documentation/case.md

+| `bc_[x,y,z]%alpha_rho_in`     | Real Array | Inflow density |
+| `bc_[x,y,z]%alpha_in`         | Real Array | Inflow void fraction |
+
+*: This boundary condition can be used for subsonic inflow (`bc_[x,y,z]%[beg,end]` = -7) and subsonic outflow (`bc_[x,y,z]%[beg,end]` = -8) characteristic boundary conditions. These are based on [Pirozzoli (2013)](references.md#Pirozzoli13). This enables to provide inflow and outflow conditions outside the computational domain. The parameters associated with this feature are listed in table [Generalized Characteristic Boundary conditions](#generalized-characteristic-boundary-conditions).


where does the * go?

it doesn't seem to be attached to any variables

The parameters associated with this feature are listed in table Generalized Characteristic Boundary conditions.

This seems to be obvious given that the text is right below the table, no?

sbryngelson · 2024-11-12T00:46:53Z

src/simulation/m_cbc.fpp

@@ -102,6 +102,18 @@ module m_cbc
    integer :: cbc_dir, cbc_loc
    !$acc declare create(dj, bcxb, bcxe, bcyb, bcye, bczb, bcze, cbc_dir, cbc_loc)

+    real(kind(0d0)) :: ux_in, ux_out, vx_in, vx_out, wx_in, wx_out, presx_in, presx_out, Delx_in, Delx_out


you need docstrings/doxygen and comments. also this is a very messy way to implement this. you have [u/v/w][x/y/z]_[in/out] for a total of 18 variables. these could easily be stored in a single 3,3,2 array.

sbryngelson · 2024-11-12T00:47:05Z

src/simulation/m_cbc.fpp

+    !$acc declare create(uy_in, uy_out, vy_in, vy_out, wy_in, wy_out, presy_in, presy_out, Dely_in, Dely_out)
+    !$acc declare create(uz_in, uz_out, vz_in, vz_out, wz_in, wz_out, presz_in, presz_out, Delz_in, Delz_out)
+
+    real(kind(0d0)), allocatable, dimension(:) :: alpha_rhox_in, alphax_in


same comment as above

sbryngelson · 2024-11-12T00:47:30Z

src/simulation/m_cbc.fpp

+
+        #:for CBC_DIR, XYZ in [(1, 'x'), (2, 'y'), (3, 'z')]
+            if (${CBC_DIR}$ == 1) then
+                u${XYZ}$_in = bc_${XYZ}$%u_in


why use u/v/w when you can use u(1,2,3)?

sbryngelson · 2024-11-12T00:49:01Z

src/simulation/m_start_up.fpp

@@ -1465,6 +1465,9 @@ contains
        !$acc update device(bc_y%vb1, bc_y%vb2, bc_y%vb3, bc_y%ve1, bc_y%ve2, bc_y%ve3)
        !$acc update device(bc_z%vb1, bc_z%vb2, bc_z%vb3, bc_z%ve1, bc_z%ve2, bc_z%ve3)

+        !$acc update device(bc_x%grcbc_in, bc_x%grcbc_out, bc_x%grcbc_vel_out)


9 variables when you could just have one?

sbryngelson

lots of messy verbose code that can be easily cleaned up. also only three new tests? should have more.

anandrdbz · 2024-11-14T23:03:58Z

@sbryngelson I've made most of the suggested changes, the only repetition is in the bc variables, but this is how it's done for all other cases (separate variables for bc_x, bc_y and bc_z). However in the CBC module this is transformed into an array of size (3) as suggested. Most of the verbosity in the code should've been removed now. And I've added 3 more test cases; these should cover every possible line I've added in CBC, so further tests are unnecessary. I've added a few more comments in the CBC file as well

sbryngelson · 2024-11-14T23:38:18Z

@sbryngelson I've made most of the suggested changes, the only repetition is in the bc variables, but this is how it's done for all other cases (separate variables for bc_x, bc_y and bc_z). However in the CBC module this is transformed into an array of size (3) as suggested. Most of the verbosity in the code should've been removed now. And I've added 3 more test cases; these should cover every possible line I've added in CBC, so further tests are unnecessary. I've added a few more comments in the CBC file as well

Thanks I will review again shortly!

anandrdbz · 2024-11-16T22:04:29Z

@sbryngelson For some reason the job on Phoenix has been in queue for 2 days, every other test has passed and I've checked the test suite on phoenix on an interactive node

sbryngelson · 2024-11-16T22:17:45Z

@anandrdbz Yes those nodes are backed up big time for some reason. I'll merge it once it passes you don't have to do anything.

Co-authored-by: Anand <anand@lawn-128-61-28-98.lawn.gatech.edu> Co-authored-by: Anand <anand@lawn-128-61-77-225.lawn.gatech.edu> Co-authored-by: Anand <anand@ipsec-10-2-68-64.vpn.gatech.edu> Co-authored-by: Anand Radhakrishnan <aradhakr34@login05.frontier.olcf.ornl.gov> Co-authored-by: Anand <anand@Anands-MacBook-Pro.local>

GRCBC impl in 3 dims

2b79939

anandrdbz requested review from sbryngelson and henryleberre as code owners November 6, 2024 22:14

Anand and others added 12 commits November 6, 2024 17:15

Case file for steady shock with CBC inflow

1d4bdcc

Merge conflict fix

740eaad

Merge conflict fix

591d390

MERGE CONFLICT

d8db716

FORMAT + TESTS PASS

510b67d

Regression Tests and Checker

dce7d11

Zero Circulation Vortex with subsonic Inflow/Outflow

f461885

2D acoustic pulse with subsonic pressure outflow

2233240

Update case.md

c8957b3

Update references.md

6e4e2df

GPU Fixes (passes on frontier and phoenix)

66ed3a5

Merge branch 'GRCBC' of https://github.com/anandrdbz/MFC into GRCBC

c032ee9

Anand added 3 commits November 7, 2024 22:02

Removing whitespace + regression test bug caught in toolchain (lint)

9a2b3dd

Removing whitespace (final)

e98a27d

Formatting

f83d8b2

Anand added 2 commits November 11, 2024 09:31

FIXING MERGE CONFLICTS

f19aac4

FORMAT

c14e218

sbryngelson requested a review from wilfonba November 11, 2024 21:11

wilfonba reviewed Nov 12, 2024

View reviewed changes

examples/2D_acoustic_pulse/case.py Show resolved Hide resolved

examples/2D_ibm_steady_shock/case.py Show resolved Hide resolved

examples/2D_zero_circ_vortex/case.py Show resolved Hide resolved

sbryngelson reviewed Nov 12, 2024

View reviewed changes

sbryngelson requested changes Nov 12, 2024

View reviewed changes

sbryngelson added the enhancement New feature or request label Nov 13, 2024

anandrdbz and others added 3 commits November 14, 2024 14:23

Merge branch 'master' into GRCBC

e74ddf0

Removing verbose code

ead643f

removing verbose code

48209c1

frontier build issue

3792b88

sbryngelson approved these changes Nov 17, 2024

View reviewed changes

sbryngelson merged commit ff6e3e0 into MFlowCode:master Nov 17, 2024
25 checks passed

sbryngelson mentioned this pull request Nov 17, 2024

(Atomic) Boundary Condition Patches (pt. 1/2) #737

Draft

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GRCBC #698

GRCBC #698

anandrdbz commented Nov 6, 2024 •

edited

Loading

codecov bot commented Nov 8, 2024 •

edited

Loading

sbryngelson commented Nov 8, 2024

anandrdbz commented Nov 11, 2024

sbryngelson commented Nov 11, 2024

sbryngelson Nov 12, 2024

sbryngelson Nov 12, 2024

sbryngelson Nov 12, 2024

sbryngelson Nov 12, 2024

sbryngelson Nov 12, 2024

sbryngelson Nov 12, 2024

sbryngelson Nov 12, 2024

sbryngelson left a comment

anandrdbz commented Nov 14, 2024

sbryngelson commented Nov 14, 2024

anandrdbz commented Nov 16, 2024

sbryngelson commented Nov 16, 2024 •

edited

Loading

GRCBC #698

GRCBC #698

Conversation

anandrdbz commented Nov 6, 2024 • edited Loading

Description

Type of change

Scope

How Has This Been Tested?

Checklist

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

codecov bot commented Nov 8, 2024 • edited Loading

Codecov Report

sbryngelson commented Nov 8, 2024

anandrdbz commented Nov 11, 2024

sbryngelson commented Nov 11, 2024

sbryngelson Nov 12, 2024

Choose a reason for hiding this comment

sbryngelson Nov 12, 2024

Choose a reason for hiding this comment

sbryngelson Nov 12, 2024

Choose a reason for hiding this comment

sbryngelson Nov 12, 2024

Choose a reason for hiding this comment

sbryngelson Nov 12, 2024

Choose a reason for hiding this comment

sbryngelson Nov 12, 2024

Choose a reason for hiding this comment

sbryngelson Nov 12, 2024

Choose a reason for hiding this comment

sbryngelson left a comment

Choose a reason for hiding this comment

anandrdbz commented Nov 14, 2024

sbryngelson commented Nov 14, 2024

anandrdbz commented Nov 16, 2024

sbryngelson commented Nov 16, 2024 • edited Loading

anandrdbz commented Nov 6, 2024 •

edited

Loading

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

codecov bot commented Nov 8, 2024 •

edited

Loading

sbryngelson commented Nov 16, 2024 •

edited

Loading