Add rockets to website #982

wilfonba · 2025-08-10T19:39:36Z

User description

Add a visualization of multijet rocket to the website

PR Type

Enhancement, Bug fix, Tests

Description

• Added comprehensive downsampling support across all MFC modules with 3D data averaging and upsampling capabilities
• Implemented Frontier unified memory management with HIP memory allocation and wall time performance tracking
• Fixed volume fraction handling bugs for single-fluid IGR (Interface Ghost Reconstruction) cases
• Added viscous multi-jet rocket simulation visualization to the website documentation
• Updated test metadata files with new timestamps and configuration changes
• Enhanced data I/O interfaces with optional IB (Immersed Boundary) parameters and downsampling support
• Added MPI support for downsampled data with strided access patterns
• Improved compiler support with Cray Fortran optimization directives and unified memory flags

Diagram Walkthrough

flowchart LR
  A["Downsampling Core"] --> B["Data I/O Enhancement"]
  A --> C["MPI Support"]
  D["Frontier Memory"] --> E["Performance Tracking"]
  F["IGR Fixes"] --> G["Volume Fraction Handling"]
  H["Website Update"] --> I["Rocket Visualization"]
  B --> J["Optional IB Parameters"]
  C --> K["Strided Data Access"]

File Walkthrough

Relevant files

Enhancement

19 files

m_time_steppers.fpp `Frontier unified memory support and performance tracking` src/simulation/m_time_steppers.fpp • Added FRONTIER_UNIFIED memory management with HIP memory allocation and pointer mapping • Implemented wall time tracking and averaging across all time-stepping routines • Added compiler optimization directives for Cray Fortran compiler • Modified time-stepping algorithms to handle unified memory architecture	+253/-43
m_data_output.fpp `Data output interface improvements and downsampling support` src/pre_process/m_data_output.fpp • Modified abstract interface to make IB-related parameters optional • Added downsampling support with `q_cons_temp` temporary arrays • Updated data writing functions to handle optional IB markers and levelset data • Added initialization and cleanup for downsampled data structures	+122/-48
m_start_up.fpp `Simulation startup with downsampling and performance enhancements` src/simulation/m_start_up.fpp • Added downsampling support with temporary arrays and upsampling functionality • Enhanced performance metrics display with wall time averaging • Modified data reading/writing to handle downsampled data • Updated GPU variable initialization for downsampling compatibility	+98/-29
m_data_output.fpp `Simulation data output with downsampling capabilities` src/simulation/m_data_output.fpp • Added downsampling support with `q_cons_temp` arrays • Modified data writing functions to handle downsampled data • Updated memory allocation for downsampled arrays • Added conditional allocation based on `run_time_info` flag	+105/-34
m_data_input.f90 `Post-processing data input with downsampling support` src/post_process/m_data_input.f90 • Added downsampling support for parallel data file reading • Implemented MPI vector types for strided data access • Added `q_cons_temp` temporary arrays for downsampled data • Modified data reading routines to handle downsampled grid sizes	+76/-3
m_global_parameters.fpp `Global parameters update for downsampling and IGR fixes` src/pre_process/m_global_parameters.fpp • Added `down_sample` boolean parameter • Fixed volume fraction indexing logic for IGR method • Updated coordinate bounds configuration with IGR order parameter • Modified MPI data allocation to skip when downsampling is enabled	+26/-13
m_compute_levelset.fpp `Levelset computation with optional parameters` src/pre_process/m_compute_levelset.fpp • Made levelset and levelset_norm parameters optional in all levelset computation functions • Updated function signatures to support optional IB-related data	+21/-21
m_helper.fpp `Data downsampling and upsampling helper functions` src/common/m_helper.fpp • Added `s_downsample_data` subroutine for 3D data downsampling with 27-point averaging • Added `s_upsample_data` subroutine for interpolating downsampled data back to full resolution	+86/-1
m_global_parameters.fpp `Global parameters with wall time tracking and downsampling` src/simulation/m_global_parameters.fpp • Replaced `time` variable with `wall_time` and `wall_time_avg` for performance tracking • Added `down_sample` parameter with GPU declaration • Fixed volume fraction indexing for IGR method • Updated coordinate bounds configuration	+25/-12
m_global_parameters.fpp `Post-processing global parameters with downsampling` src/post_process/m_global_parameters.fpp • Added `down_sample` boolean parameter • Fixed volume fraction indexing logic for IGR method • Updated MPI data allocation for downsampling support	+20/-8
m_patches.fpp `Patch application with optional IB parameters` src/pre_process/m_patches.fpp • Made IB-related parameters optional in patch application functions • Updated function calls to use optional parameters for levelset computations	+11/-10
m_initial_condition.fpp `Initial condition setup with conditional IB support` src/pre_process/m_initial_condition.fpp • Made IB-related allocations conditional based on `ib` flag • Updated patch application to handle optional IB parameters	+16/-7
m_boundary_common.fpp `Boundary common interface improvements` src/common/m_boundary_common.fpp • Added `intent(in)` declarations to function parameters for better interface clarity	+7/-7
m_mpi_common.fpp `MPI common with downsampling data initialization` src/common/m_mpi_common.fpp • Added `s_initialize_mpi_data_ds` subroutine for downsampled MPI data initialization	+51/-0
m_start_up.f90 `Post-processing startup with downsampling support` src/post_process/m_start_up.f90 • Added downsampling support to input file reading and grid size calculations	+9/-2
m_helper_basic.fpp `Helper basic functions with IGR order support` src/common/m_helper_basic.fpp • Updated coordinate bounds configuration to include `igr_order` parameter • Modified buffer size calculation for IGR method	+6/-7
m_checker.fpp `Input validation for downsampling feature` src/pre_process/m_checker.fpp • Added validation checks for downsampling requirements and constraints	+8/-0
index.html `Add viscous multi-jet rocket simulation to website` docs/index.html • Added new simulation entry for "Viscous multi-jet rocket" • Included simulation details: Frontier system, 12k MI250X GPUs, 1.5h runtime • Added Vimeo video link for the rocket visualization	+1/-0
CMakeLists.txt `Add Frontier unified memory support for Cray compiler` CMakeLists.txt • Added Frontier Unified Memory support for Cray Fortran compiler • Added conditional compilation flag `-DFRONTIER_UNIFIED` when `MFC_Unified` is enabled • Included support within existing Cray compiler configuration block	+6/-0

Bug fix

3 files

m_variables_conversion.fpp `Variable conversion fixes for IGR method` src/common/m_variables_conversion.fpp • Fixed volume fraction handling for single-fluid IGR cases • Added conditional logic to skip volume fraction loops when not needed	+14/-10
m_assign_variables.fpp `Variable assignment fixes for IGR method` src/pre_process/m_assign_variables.fpp • Added conditional logic to skip volume fraction assignment for single-fluid IGR cases	+20/-14
m_sim_helpers.fpp `Simulation helpers IGR fix` src/simulation/m_sim_helpers.fpp • Fixed volume fraction handling for single-fluid IGR cases	+1/-1

Configuration changes

7 files

m_mpi_proxy.fpp `MPI proxy updates for new parameters` src/pre_process/m_mpi_proxy.fpp • Added `igr_order` and `down_sample` to MPI broadcast parameters	+4/-3
m_mpi_proxy.fpp `Post-processing MPI proxy with downsampling parameter` src/post_process/m_mpi_proxy.fpp • Added `down_sample` to MPI broadcast parameters	+3/-2
m_start_up.fpp `Pre-processing startup with downsampling parameter` src/pre_process/m_start_up.fpp • Added `down_sample` parameter to input file reading • Updated data file writing to handle optional IB parameters	+6/-2
m_mpi_proxy.fpp `Simulation MPI proxy with downsampling parameter` src/simulation/m_mpi_proxy.fpp • Added `down_sample` to MPI broadcast parameters	+2/-2
case_dicts.py `Case dictionary updates for new parameters` toolchain/mfc/run/case_dicts.py • Added `down_sample` parameter to case dictionary definitions • Added domain boundary parameters for x, y, z coordinates	+5/-0
frontier.mako `Frontier template with unified memory support` toolchain/templates/frontier.mako • Added conditional export of `CRAY_ACC_USE_UNIFIED_MEM` environment variable	+4/-0
.typos.toml `Typos configuration update` .typos.toml • Added "HSA" to the list of allowed words	+1/-0

Tests

13 files

golden-metadata.txt `Test metadata update` tests/A57E30FE/golden-metadata.txt • Updated test metadata with new timestamps and environment information	+10/-43
golden-metadata.txt `Update test metadata with new timestamps and configuration` tests/106C0BE6/golden-metadata.txt • Updated file creation timestamp from July 8 to July 24, 2025 • Changed test invocation from batch mode to single test execution • Updated Git commit hash and branch from IGRMergeOne to FrontierUVM • Modified hostname from VPN address to local MacBook • Reordered build configuration sections and updated environment variables	+22/-55
golden-metadata.txt `Update test metadata with new timestamps and configuration` tests/CEAF553A/golden-metadata.txt • Updated file creation timestamp from July 8 to July 24, 2025 • Changed test invocation from batch mode to single test execution • Updated Git commit hash and branch from IGRMergeOne to FrontierUVM • Modified hostname from VPN address to local MacBook • Reordered build configuration sections and updated environment variables	+22/-55
golden-metadata.txt `Update test metadata with new timestamps and configuration` tests/6077374F/golden-metadata.txt • Updated file creation timestamp from July 8 to July 24, 2025 • Changed test invocation from batch mode to single test execution • Updated Git commit hash and branch from IGRMergeOne to FrontierUVM • Modified hostname from VPN address to local MacBook • Removed documentation section and reordered remaining sections	+20/-53
golden-metadata.txt `Update test metadata with new timestamps and configuration` tests/AE3FC5CB/golden-metadata.txt • Updated file creation timestamp from July 8 to July 24, 2025 • Changed test invocation from batch mode to single test execution • Updated Git commit hash and branch from IGRMergeOne to FrontierUVM • Modified hostname from VPN address to local MacBook • Reordered build configuration sections and updated environment variables	+19/-52
golden-metadata.txt `Update test metadata with new timestamps and configuration` tests/0045D9F8/golden-metadata.txt • Updated file creation timestamp from July 8 to July 24, 2025 • Changed test invocation from batch mode to single test execution • Updated Git commit hash and branch from IGRMergeOne to FrontierUVM • Modified hostname from VPN address to local MacBook • Reordered build configuration sections and updated environment variables	+19/-52
golden-metadata.txt `Update test metadata with new timestamps and configuration` tests/4440D46B/golden-metadata.txt • Updated file creation timestamp from July 8 to July 24, 2025 • Changed test invocation from batch mode to single test execution • Updated Git commit hash and branch from IGRMergeOne to FrontierUVM • Modified hostname from VPN address to local MacBook • Reordered build configuration sections and updated environment variables	+19/-52
golden-metadata.txt `Update test metadata with new timestamps and configuration` tests/C7A6B609/golden-metadata.txt • Updated file creation timestamp from July 8 to July 24, 2025 • Changed test invocation from batch mode to single test execution • Updated Git commit hash and branch from IGRMergeOne to FrontierUVM • Modified hostname from VPN address to local MacBook • Reordered build configuration sections and updated environment variables	+19/-52
golden-metadata.txt `Update test metadata with new timestamps and configuration` tests/E8979E4A/golden-metadata.txt • Updated file creation timestamp from July 8 to July 24, 2025 • Changed test invocation from batch mode to single test execution • Updated Git commit hash and branch from IGRMergeOne to FrontierUVM • Modified hostname from VPN address to local MacBook • Reordered build configuration sections and updated environment variables	+16/-49
golden-metadata.txt `Update test metadata with new timestamps and configuration` tests/AFBCBDFA/golden-metadata.txt • Updated file creation timestamp from July 8 to July 24, 2025 • Changed test invocation from batch mode to single test execution • Updated Git commit hash and branch from IGRMergeOne to FrontierUVM • Modified hostname from VPN address to local MacBook • Reordered build configuration sections and updated environment variables	+16/-49
golden-metadata.txt `Update test metadata with new timestamps and configuration` tests/37DDE283/golden-metadata.txt • Updated file creation timestamp from July 8 to July 24, 2025 • Changed test invocation from batch mode to single test execution • Updated Git commit hash and branch from IGRMergeOne to FrontierUVM • Modified hostname from VPN address to local MacBook • Removed documentation section and reordered remaining sections	+14/-47
golden-metadata.txt `Update test metadata with new timestamps and configuration` tests/EB58AF7F/golden-metadata.txt • Updated file creation timestamp from July 8 to July 24, 2025 • Changed test invocation from batch mode to single test execution • Updated Git commit hash and branch from IGRMergeOne to FrontierUVM • Modified hostname from VPN address to local MacBook • Removed simulation section and reordered remaining sections	+14/-47
golden-metadata.txt `Update test metadata with new timestamps and configuration` tests/E49EF7B6/golden-metadata.txt • Updated file creation timestamp from July 8 to July 25, 2025 • Updated Git commit hash from `945bc22` to `bc21e60` • Modified hostname from VPN address to local MacBook • Removed documentation section and reordered remaining sections	+12/-45

Formatting

1 files

modules `Clean up trailing whitespace in module configurations` toolchain/modules • Removed trailing whitespace from Frontier GPU configuration line • Removed trailing whitespace from Carpenter configuration line	+2/-2

Additional files

13 files

golden.txt	+1/-3
golden.txt	+1/-3
golden.txt	+1/-3
golden.txt	+1/-3
golden.txt	+1/-3
golden.txt	+1/-3
golden.txt	+1/-3
golden.txt	+1/-3
golden.txt	+1/-3
golden.txt	+1/-3
golden.txt	+1/-3
golden.txt	+1/-3
golden.txt	+1/-3

Copilot

Pull Request Overview

This PR adds GPU unified memory support and parameter configuration enhancements for rocket visualization functionality. The main purpose appears to be enabling unified memory management for GPU acceleration and expanding parameter flexibility for domain configuration.

Key changes include:

Addition of GPU unified memory support for Frontier supercomputer configurations
Extension of parameter dictionaries to support dynamic domain boundary parameters
Update of test golden files with new baseline values

Reviewed Changes

Copilot reviewed 45 out of 57 changed files in this pull request and generated 1 comment.

File	Description
toolchain/templates/frontier.mako	Adds conditional export of `CRAY_ACC_USE_UNIFIED_MEM=1` for unified memory support
toolchain/mfc/run/case_dicts.py	Adds `down_sample` parameter and dynamic domain boundary parameters for x,y,z components
tests/EB58AF7F/golden.txt	Updates test baseline with 2 lines removed from numerical output data
tests/EB58AF7F/golden-metadata.txt	Updates test metadata with new timestamps, git commit, and build configuration

Copilot · 2025-08-10T19:40:05Z

toolchain/mfc/run/case_dicts.py

    'output_partial_domain': ParamType.LOG,
    'bubbles_lagrange': ParamType.LOG,
 })



The parameter naming pattern creates potentially confusing names like 'x_a' and 'y_b'. Consider adding a comment explaining what these generated parameters represent or using more descriptive naming patterns.

Suggested change

# The following loop generates parameter names like 'x_a', 'y_b', 'z_domain%beg', etc.

# 'cmp' stands for the spatial component ('x', 'y', 'z'), and 'prepend' indicates the domain boundary or variable ('domain%beg', 'domain%end', 'a', 'b').

# These parameters are used to specify real-valued settings for each spatial direction and domain boundary.

qodo-merge-pro · 2025-08-10T19:41:01Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 5 🔵🔵🔵🔵🔵
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Possible Issue Under FRONTIER_UNIFIED paths, the new ping-pong logic uses a 'dest' selector to choose which q_cons_ts buffer holds the valid state. Please verify that all subsequent calls in each RK stage consistently use q_cons_ts(dest)%vf (e.g., body forces, filters, relaxation, IBM) and that no remaining calls still assume fixed indices, to avoid reading stale data. end do end if if (bodyForces) call s_apply_bodyforces(q_cons_ts(dest)%vf, q_prim_vf, rhs_vf, dt) if (grid_geometry == 3) call s_apply_fourier_filter(q_cons_ts(dest)%vf) if (model_eqns == 3 .and. (.not. relax)) then call s_pressure_relaxation_procedure(q_cons_ts(dest)%vf) end if if (adv_n) call s_comp_alpha_from_n(q_cons_ts(dest)%vf) if (ib) then if (qbmm .and. .not. polytropic) then call s_ibm_correct_state(q_cons_ts(dest)%vf, q_prim_vf, pb_ts(2)%sf, mv_ts(2)%sf) else call s_ibm_correct_state(q_cons_ts(dest)%vf, q_prim_vf) end if end if ! Stage 2 of 2 call s_compute_rhs(q_cons_ts(dest)%vf, q_T_sf, q_prim_vf, bc_type, rhs_vf, pb_ts(2)%sf, rhs_pb, mv_ts(2)%sf, rhs_mv, t_step, time_avg, 2) if (bubbles_lagrange .and. .not. adap_dt) call s_update_lagrange_tdv_rk(stage=2) #ifdef FRONTIER_UNIFIED $:GPU_PARALLEL_LOOP(collapse=4) do i = 1, sys_size do l = 0, p do k = 0, n do j = 0, m q_cons_ts(1)%vf(i)%sf(j, k, l) = & (q_cons_ts(2)%vf(i)%sf(j, k, l) & + q_cons_ts(1)%vf(i)%sf(j, k, l) & + dtrhs_vf(i)%sf(j, k, l))/4._wp end do end do end do end do dest = 1 ! Result in q_cons_ts(1)%vf #else $:GPU_PARALLEL_LOOP(collapse=4) do i = 1, sys_size do l = 0, p do k = 0, n do j = 0, m q_cons_ts(1)%vf(i)%sf(j, k, l) = & (q_cons_ts(1)%vf(i)%sf(j, k, l) & + q_cons_ts(2)%vf(i)%sf(j, k, l) & + dtrhs_vf(i)%sf(j, k, l))/2._wp end do end do end do end do dest = 1 ! Result in q_cons_ts(1)%vf #endif if (qbmm .and. (.not. polytropic)) then $:GPU_PARALLEL_LOOP(collapse=5) do i = 1, nb do l = 0, p do k = 0, n do j = 0, m do q = 1, nnode pb_ts(1)%sf(j, k, l, q, i) = & (pb_ts(1)%sf(j, k, l, q, i) & + pb_ts(2)%sf(j, k, l, q, i) & + dtrhs_pb(j, k, l, q, i))/2._wp end do end do end do end do end do end if if (qbmm .and. (.not. polytropic)) then $:GPU_PARALLEL_LOOP(collapse=5) do i = 1, nb do l = 0, p do k = 0, n do j = 0, m do q = 1, nnode mv_ts(1)%sf(j, k, l, q, i) = & (mv_ts(1)%sf(j, k, l, q, i) & + mv_ts(2)%sf(j, k, l, q, i) & + dtrhs_mv(j, k, l, q, i))/2._wp end do end do end do end do end do end if if (bodyForces) call s_apply_bodyforces(q_cons_ts(dest)%vf, q_prim_vf, rhs_vf, 2._wpdt/3._wp) if (grid_geometry == 3) call s_apply_fourier_filter(q_cons_ts(dest)%vf) if (model_eqns == 3 .and. (.not. relax)) then call s_pressure_relaxation_procedure(q_cons_ts(dest)%vf) end if if (adv_n) call s_comp_alpha_from_n(q_cons_ts(dest)%vf) if (ib) then if (qbmm .and. .not. polytropic) then call s_ibm_correct_state(q_cons_ts(dest)%vf, q_prim_vf, pb_ts(1)%sf, mv_ts(1)%sf) else call s_ibm_correct_state(q_cons_ts(dest)%vf, q_prim_vf) end if end if Optional Args Use* Signatures now mark ib_markers, levelset, and levelset_norm as optional but code writes these files when ib is true without checking presence. Ensure these optionals are present before dereferencing to avoid runtime errors when IB data is not initialized. end if end if if (ib) then ! Outputting IB Markers file_loc = trim(t_step_dir)//'/ib.dat' open (1, FILE=trim(file_loc), FORM='unformatted', STATUS=status) write (1) ib_markers%sf close (1) do i = 1, num_ibs if (patch_ib(i)%geometry == 4) then file_loc = trim(t_step_dir)//'/airfoil_u.dat' open (1, FILE=trim(file_loc), FORM='unformatted', STATUS=status) write (1) airfoil_grid_u(1:Np) close (1) file_loc = trim(t_step_dir)//'/airfoil_l.dat' open (1, FILE=trim(file_loc), FORM='unformatted', STATUS=status) write (1) airfoil_grid_l(1:Np) close (1) end if end do ! Outtputting Levelset Info file_loc = trim(t_step_dir)//'/levelset.dat' open (1, FILE=trim(file_loc), FORM='unformatted', STATUS=status) write (1) levelset%sf close (1) file_loc = trim(t_step_dir)//'/levelset_norm.dat' open (1, FILE=trim(file_loc), FORM='unformatted', STATUS=status) write (1) levelset_norm%sf close (1) end if MPI Type Cleanup New MPI_TYPE_VECTOR and FILE_SET_VIEW are created for downsampled coordinate reads but not freed. Confirm MPI derived datatypes are freed (MPI_TYPE_FREE) to prevent resource leaks in long-running post-processing. allocate (y_cb_glb(-1:n_glb)) allocate (z_cb_glb(-1:p_glb)) if (down_sample) then stride = 3 else stride = 1 end if ! Read in cell boundary locations in x-direction file_loc = trim(case_dir)//'/restart_data'//trim(mpiiofs)//'x_cb.dat' inquire (FILE=trim(file_loc), EXIST=file_exist) if (file_exist) then data_size = m_glb + 2 call MPI_FILE_OPEN(MPI_COMM_WORLD, file_loc, MPI_MODE_RDONLY, mpi_info_int, ifile, ierr) call MPI_TYPE_VECTOR(data_size, 1, stride, mpi_p, filetype, ierr) call MPI_TYPE_COMMIT(filetype, ierr) offset = 0 call MPI_FILE_SET_VIEW(ifile, offset, mpi_p, filetype, 'native', mpi_info_int, ierr) call MPI_FILE_READ(ifile, x_cb_glb, data_size, mpi_p, status, ierr) call MPI_FILE_CLOSE(ifile, ierr) else call s_mpi_abort('File '//trim(file_loc)//' is missing. Exiting.') end if ! Assigning local cell boundary locations x_cb(-1:m) = x_cb_glb((start_idx(1) - 1):(start_idx(1) + m)) ! Computing the cell width distribution dx(0:m) = x_cb(0:m) - x_cb(-1:m - 1) ! Computing the cell center location x_cc(0:m) = x_cb(-1:m - 1) + dx(0:m)/2._wp if (n > 0) then ! Read in cell boundary locations in y-direction file_loc = trim(case_dir)//'/restart_data'//trim(mpiiofs)//'y_cb.dat' inquire (FILE=trim(file_loc), EXIST=file_exist) if (file_exist) then data_size = n_glb + 2 call MPI_FILE_OPEN(MPI_COMM_WORLD, file_loc, MPI_MODE_RDONLY, mpi_info_int, ifile, ierr) call MPI_TYPE_VECTOR(data_size, 1, stride, mpi_p, filetype, ierr) call MPI_TYPE_COMMIT(filetype, ierr) offset = 0 call MPI_FILE_SET_VIEW(ifile, offset, mpi_p, filetype, 'native', mpi_info_int, ierr) call MPI_FILE_READ(ifile, y_cb_glb, data_size, mpi_p, status, ierr) call MPI_FILE_CLOSE(ifile, ierr) else call s_mpi_abort('File '//trim(file_loc)//' is missing. Exiting.') end if ! Assigning local cell boundary locations y_cb(-1:n) = y_cb_glb((start_idx(2) - 1):(start_idx(2) + n)) ! Computing the cell width distribution dy(0:n) = y_cb(0:n) - y_cb(-1:n - 1) ! Computing the cell center location y_cc(0:n) = y_cb(-1:n - 1) + dy(0:n)/2._wp if (p > 0) then ! Read in cell boundary locations in z-direction file_loc = trim(case_dir)//'/restart_data'//trim(mpiiofs)//'z_cb.dat' inquire (FILE=trim(file_loc), EXIST=file_exist) if (file_exist) then data_size = p_glb + 2 call MPI_FILE_OPEN(MPI_COMM_WORLD, file_loc, MPI_MODE_RDONLY, mpi_info_int, ifile, ierr) call MPI_TYPE_VECTOR(data_size, 1, stride, mpi_p, filetype, ierr) call MPI_TYPE_COMMIT(filetype, ierr) offset = 0 call MPI_FILE_SET_VIEW(ifile, offset, mpi_p, filetype, 'native', mpi_info_int, ierr) call MPI_FILE_READ(ifile, z_cb_glb, data_size, mpi_p, status, ierr) call MPI_FILE_CLOSE(ifile, ierr) else

qodo-merge-pro · 2025-08-10T19:42:04Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Guard unified-memory path correctness The FRONTIER_UNIFIED path introduces HIP managed/device pools, pointer mapping, and dest-based flow with comments forbidding atomics, but there’s no global safeguard ensuring kernels and library calls never use atomics on these allocations or that HSA_XNACK is consistently enforced at runtime. Add a centralized capability flag and runtime assertions to verify memory properties and forbid atomic use on unified-memory-backed arrays, or provide a fallback path, to avoid subtle correctness/performance bugs on Frontier and other HIP/OpenACC toolchains. Examples: src/simulation/m_time_steppers.fpp [128-130] ! CCE see it can access this and will leave it on the host. It will stay on the host so long as HSA_XNACK=1 ! NOTE: WE CANNOT DO ATOMICS INTO THIS MEMORY. We have to change a property to use atomics here ! Otherwise leaving this as fine-grained will actually help performance since it can't be cached in GPU L2 Solution Walkthrough: Before: ! In m_time_steppers.fpp subroutine s_initialize_time_steppers_module ... #ifdef FRONTIT_UNIFIED ! NOTE: WE CANNOT DO ATOMICS INTO THIS MEMORY. ! It will stay on the host so long as HSA_XNACK=1 call hipCheck(hipMallocManaged(q_cons_ts_pool_host, ...)) ... #endif ... end subroutine After: ! In a new or existing helper module subroutine s_check_frontier_unified_env() character(len=10) :: hsa_xnack_val call get_environment_variable("HSA_XNACK", hsa_xnack_val) if (trim(hsa_xnack_val) /= "1") then call s_mpi_abort("FRONTIER_UNIFIED requires HSA_XNACK=1") end if end subroutine ! In m_time_steppers.fpp subroutine s_initialize_time_steppers_module ... #ifdef FRONTIER_UNIFIED call s_check_frontier_unified_env() ! Atomics are forbidden on this memory. ! Kernels using this memory should be checked. call hipCheck(hipMallocManaged(q_cons_ts_pool_host, ...)) #endif ... end subroutine Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical design weakness in the new `FRONTIER_UNIFIED` memory model, where correctness relies on developer conventions and environment settings mentioned only in comments, which is fragile and can lead to hard-to-debug errors.	High
Possible issue	Fix MPI offset variables The wrong variable is used when setting `n_MOK` and `p_MOK`, which will corrupt MPI views/offsets and can cause read failures or wrong data. Use the corresponding global sizes for each direction. src/simulation/m_start_up.fpp [676-677] ! Resize some integers so MPI can read even the biggest file m_MOK = int(m_glb_read + 1, MPI_OFFSET_KIND) -n_MOK = int(m_glb_read + 1, MPI_OFFSET_KIND) -p_MOK = int(m_glb_read + 1, MPI_OFFSET_KIND) +n_MOK = int(n_glb_read + 1, MPI_OFFSET_KIND) +p_MOK = int(p_glb_read + 1, MPI_OFFSET_KIND) Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical copy-paste error where `n_MOK` and `p_MOK` were incorrectly initialized using `m_glb_read`, which would cause incorrect MPI data reads.	High
Possible issue	Clamp non-physical alpha_rho Guard against non-physical values in `alpha_rho(1)` when forcing `alpha(1)=1._wp`. Add a check to clamp `alpha_rho(1)` to be non-negative (and optionally consistent with density) to avoid propagating invalid states. src/simulation/m_sim_helpers.fpp [110-111] -alpha_rho(1) = q_prim_vf(contxb)%sf(j, k, l) +alpha_rho(1) = max(0._wp, q_prim_vf(contxb)%sf(j, k, l)) alpha(1) = 1._wp Suggestion importance[1-10]: 6 __ Why: This is a good defensive programming suggestion that improves the robustness of the simulation by preventing `alpha_rho(1)` from taking non-physical negative values.	Low
General	Correct separate intents for arguments `q_cons_temp` is an array of derived types with allocatable `sf` components; declaring it as a simple `scalar_field` array is correct, but the argument intent should be `intent(inout)` only for the container, not both variables in one declaration. Split declarations to avoid ambiguous association and potential compiler confusion. src/common/m_helper.fpp [630-632] subroutine s_downsample_data(q_cons_vf, q_cons_temp, m_ds, n_ds, p_ds, m_glb_ds, n_glb_ds, p_glb_ds) - type(scalar_field), dimension(sys_size), intent(inout) :: q_cons_vf, q_cons_temp + type(scalar_field), dimension(sys_size), intent(inout) :: q_cons_vf + type(scalar_field), dimension(sys_size), intent(inout) :: q_cons_temp Suggestion importance[1-10]: 4 __ Why: The suggestion improves code style and robustness by splitting the declaration of two derived-type array arguments, which is good practice.	Low
	Split combined derived-type argument declaration As with downsampling, declaring two derived-type arrays in one line with a shared intent can lead to confusion and is inconsistent with other code. Split the declarations to be explicit and safer for compilers handling derived types with allocatable components. src/common/m_helper.fpp [675-681] subroutine s_upsample_data(q_cons_vf, q_cons_temp) - type(scalar_field), intent(inout), dimension(sys_size) :: q_cons_vf, q_cons_temp + type(scalar_field), dimension(sys_size), intent(inout) :: q_cons_vf + type(scalar_field), dimension(sys_size), intent(inout) :: q_cons_temp integer :: i, j, k, l integer :: ix, iy, iz integer :: x_id, y_id, z_id real(wp), dimension(4) :: temp Suggestion importance[1-10]: 4 __ Why: The suggestion improves code style and robustness by splitting the declaration of two derived-type array arguments, which is good practice and consistent with the previous suggestion.	Low
	Make condition strictly boolean Ensure `unified` is treated as a strict boolean to prevent accidental export when it's a nonempty string. Explicitly compare to True in the condition. toolchain/templates/frontier.mako [47-49] -%if unified: +%if unified is True: export CRAY_ACC_USE_UNIFIED_MEM=1 % endif Suggestion importance[1-10]: 4 __ Why: The suggestion to use `is True` improves code clarity and robustness by making the condition on the `unified` variable explicit, preventing potential issues if it's a non-boolean "truthy" value.	Low
More

wilfonba and others added 21 commits July 23, 2025 22:28

UVM and down sample

42a3875

pre_process mem reduce

63c5fcf

MIN MEM FIXES

a47cb03

POST PROCESS FIX

84054d4

bug fixes

63ce00e

format and fix test suite

bc21e60

IGR Example golden file fix

1ba43dd

spelling fix

c110e9e

test suite works with --unified and --no-unified

6b834c7

Merge branch 'master' into FrontierUVM

55e7361

improve real time timing output

a55eecc

fix post process

303ded6

Merge remote-tracking branch 'upstream/master' into FrontierUVM

c02d32b

Merge remote-tracking branch 'upstream/master' into FrontierUVM

f25a5f2

format

5b40ed1

improve unified performance on Frontier

75ed756

Merge remote-tracking branch 'upstream/master' into FrontierUVM

752e5fc

requested changes

e184f7b

format, spelling, and missing comments

d1bfb6d

bug fixes

bf56824

add visualization

6a03a22

Copilot AI review requested due to automatic review settings August 10, 2025 19:39

wilfonba requested review from a team and sbryngelson as code owners August 10, 2025 19:39

wilfonba closed this Aug 10, 2025

Copilot AI reviewed Aug 10, 2025

View reviewed changes

qodo-merge-pro bot added the Review effort 5/5 label Aug 10, 2025

wilfonba deleted the website branch August 27, 2025 20:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add rockets to website #982

Add rockets to website #982

Uh oh!

wilfonba commented Aug 10, 2025 •

edited by qodo-merge-pro bot

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Aug 10, 2025

Uh oh!

qodo-merge-pro bot commented Aug 10, 2025

Uh oh!

qodo-merge-pro bot commented Aug 10, 2025

Examples:

Solution Walkthrough:

Before:

After:

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

+# The following loop generates parameter names like 'x_a', 'y_b', 'z_domain%beg', etc.
+# 'cmp' stands for the spatial component ('x', 'y', 'z'), and 'prepend' indicates the domain boundary or variable ('domain%beg', 'domain%end', 'a', 'b').
+# These parameters are used to specify real-valued settings for each spatial direction and domain boundary.

Add rockets to website #982

Add rockets to website #982

Uh oh!

Conversation

wilfonba commented Aug 10, 2025 • edited by qodo-merge-pro bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

PR Type

Description

Diagram Walkthrough

File Walkthrough

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Aug 10, 2025

Choose a reason for hiding this comment

Uh oh!

qodo-merge-pro bot commented Aug 10, 2025

PR Reviewer Guide 🔍

Uh oh!

qodo-merge-pro bot commented Aug 10, 2025

PR Code Suggestions ✨

Examples:

Solution Walkthrough:

Before:

After:

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

wilfonba commented Aug 10, 2025 •

edited by qodo-merge-pro bot

Loading