Tuo debug #1103
Tuo debug #1103
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughNumeric-stability guards added to levelset and boundary calculations; GPU directive simplified; derived-variables computation refactored to accept external buffers and moved from startup into the time-stepper flow; probe pressure call index adjusted; toolchain extended for OLCF Tuolumne with module templates; hipfort tag bumped to rocm-6.3.1. Changes
Sequence Diagram(s)sequenceDiagram
participant RK as TVD_RK (time steppers)
participant DV as s_compute_derived_variables
participant PROBE as s_write_probe_files / File I/O
RK->>RK: s_time_step_cycling(t_step)
alt probe_wrt and stage==1
RK->>DV: call s_compute_derived_variables(t_step, q_cons_ts(1)%vf, q_prim_ts1, q_prim_ts2)
DV-->>PROBE: provide derived/primitive buffers
PROBE->>PROBE: write probe files (uses provided buffers)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
bot
added
the
size:M
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Nitpicks 🔍
|
|
CodeAnt AI finished reviewing your PR. |
There was a problem hiding this comment.
Pull request overview
This PR adds support for the OLCF Tuolumne (Tuo) system and includes several bug fixes related to numerical stability. The main additions are a new batch template for Flux scheduler, module configuration for the Tuo system using ROCm 6.3.1, and corrections to prevent division-by-zero errors.
Changes:
- Added Tuo system support with Flux batch template and module configuration for ROCm 6.3.1
- Fixed circular module dependency between m_time_steppers and m_derived_variables by refactoring function signatures
- Added division-by-zero protection in geometry calculations using sgm_eps constant
- Corrected energy index usage in pressure computation from q_cons_vf(1) to q_cons_vf(E_idx)
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| toolchain/templates/tuo.mako | New Flux batch template for Tuo system with GPU and MPI configuration |
| toolchain/modules | Added module definitions for Tuo system (ROCm 6.3.1, Cray compilers, AMD GPU support) |
| toolchain/dependencies/CMakeLists.txt | Updated hipfort version from rocm-6.0.2 to rocm-6.3.1 for consistency |
| toolchain/bootstrap/modules.sh | Added Tuo to system selection menu with improved alignment |
| src/simulation/m_time_steppers.fpp | Added m_derived_variables module use and call to s_compute_derived_variables with required parameters |
| src/simulation/m_start_up.fpp | Removed premature call to s_compute_derived_variables and cleaned up whitespace |
| src/simulation/m_derived_variables.fpp | Removed circular dependency on m_time_steppers and updated function signature to accept parameters |
| src/simulation/m_data_output.fpp | Fixed pressure computation to use E_idx instead of hardcoded index 1 |
| src/common/m_variables_conversion.fpp | Simplified GPU_ROUTINE annotation for s_compute_speed_of_sound |
| src/common/m_model.fpp | Added division-by-zero protection using max(sgm_eps, boundary_edge(2)) |
| src/common/m_compute_levelset.fpp | Added division-by-zero protection using max(norm2(xyz_local), sgm_eps) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/common/m_model.fpp (1)
677-695:max(sgm_eps, boundary_edge(2))changes slope sign for negativeboundary_edge(2)(geometry bug).This “fix” can silently flip the tangent sign and distort normals. You already branch on
abs(boundary_edge(2)) < threshold_vector_zero, so avoid dividing before that check (or clamp by magnitude while preserving sign).Proposed fix (compute safely only when needed)
do i = 1, boundary_edge_count boundary_edge(1) = boundary_v(i, 2, 1) - boundary_v(i, 1, 1) boundary_edge(2) = boundary_v(i, 2, 2) - boundary_v(i, 1, 2) - edgetan = boundary_edge(1)/max(sgm_eps, boundary_edge(2)) - if (abs(boundary_edge(2)) < threshold_vector_zero) then - if (edgetan > 0._wp) then + if (boundary_edge(1) > 0._wp) then ynormal = -1 xnormal = 0._wp else ynormal = 1 xnormal = 0._wp end if else + edgetan = boundary_edge(1)/boundary_edge(2) initial = boundary_edge(2) ynormal = -edgetan*initial xnormal = initial end ifsrc/common/m_compute_levelset.fpp (1)
542-558: Inconsistent normalization approach: use explicit near-zero check instead of clamping.This normalization differs from
s_circle_levelsetands_sphere_levelset, which explicitly check for near-zero distances and assign a deterministic fallback (0 or a fixed direction). The current clamping viamax(norm2(xyz_local), sgm_eps)produces a non-unit normal (magnitude ~1e-16) when the distance is very small, which may cause downstream issues if unit normals are assumed.Regarding device visibility:
sgm_epsis a module parameter (defined inm_constants.fpp), so it is accessible on-device without requiring explicitcopyin. However, the normalization semantics should match the pattern used elsewhere: explicitly handle the near-zero case with a clear fallback.Proposed fix (explicit near-zero handling)
else levelset%sf(i, j, k, ib_patch_id) = dist_surface xyz_local = xyz_local*dist_surface_vec - xyz_local = xyz_local/max(norm2(xyz_local), sgm_eps) + if (f_approx_equal(norm2(xyz_local), 0._wp)) then + xyz_local = 0._wp + else + xyz_local = xyz_local/norm2(xyz_local) + end if levelset_norm%sf(i, j, k, ib_patch_id, :) = matmul(rotation, xyz_local) end if
🤖 Fix all issues with AI agents
In @src/common/m_variables_conversion.fpp:
- Around line 1379-1381: The GPU helper subroutine s_compute_speed_of_sound is
missing the explicit function_name in its $:GPU_ROUTINE decoration; update the
macro invocation for subroutine s_compute_speed_of_sound to include
function_name='s_compute_speed_of_sound' (i.e., restore the explicit naming in
the $:GPU_ROUTINE(...) parameters) so the GPU-callable helper matches the
documented decoration standard and improves clarity.
In @toolchain/bootstrap/modules.sh:
- Line 42: The "Tuolumne" module entry currently logs as "LLNL" but should be
labeled as OLCF/ORNL; update the log line that contains the string "Tuolumne
(tuo)" to use the OLCF/ORNL label instead of LLNL and relocate that line so it
sits with the other ORNL/OLCF entries (or merge it into the existing ORNL line)
in modules.sh to reflect the correct organization.
In @toolchain/templates/tuo.mako:
- Line 28: The module load command is using the wrong computer identifier ("-c
t") which doesn't match the module slug "tuo"; update the command in the
template (the line invoking "./mfc.sh load -c t -m ${'g' if gpu else 'c'}") to
use "-c tuo" so it passes the correct computer slug when invoking mfc.sh.
- Line 13: The Flux directive line contains "# flux:--setattr=thp=always" with a
missing space after the colon which breaks parsing; update that directive to
include a space after the colon so it reads "# flux: --setattr=thp=always"
(preserving the flag and value) to match the other flux directives in the
template.
🧹 Nitpick comments (1)
src/simulation/m_derived_variables.fpp (1)
119-177: Tightens_compute_derived_variablesinterface (intent(in)/ explicit shapes) to prevent accidental mutation + clarify contract.Right now
q_cons_vf,q_prim_ts1,q_prim_ts2areintent(inout)but appear read-only here (at least in this file). Also, consider usingdimension(sys_size)(or the appropriate explicit bounds) forq_cons_vfto matchs_write_probe_filesexpectations and catch mismatches early. Based on learnings, keeping interfaces strict helps avoid GPU/host aliasing surprises.Proposed signature tightening + doc touch-up
- subroutine s_compute_derived_variables(t_step, q_cons_vf, q_prim_ts1, q_prim_ts2) + subroutine s_compute_derived_variables(t_step, q_cons_vf, q_prim_ts1, q_prim_ts2) integer, intent(in) :: t_step - type(scalar_field), dimension(:), intent(inout) :: q_cons_vf - type(vector_field), dimension(:), intent(inout) :: q_prim_ts1, q_prim_ts2 + type(scalar_field), dimension(sys_size), intent(in) :: q_cons_vf + type(vector_field), dimension(:), intent(in) :: q_prim_ts1, q_prim_ts2(And add
!! @param q_cons_vf ...,!! @param q_prim_ts1 ...,!! @param q_prim_ts2 ....)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/common/m_compute_levelset.fppsrc/common/m_model.fppsrc/common/m_variables_conversion.fppsrc/simulation/m_data_output.fppsrc/simulation/m_derived_variables.fppsrc/simulation/m_start_up.fppsrc/simulation/m_time_steppers.fpptoolchain/bootstrap/modules.shtoolchain/dependencies/CMakeLists.txttoolchain/modulestoolchain/templates/tuo.mako
💤 Files with no reviewable changes (1)
- src/simulation/m_start_up.fpp
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{fpp,f90}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{fpp,f90}: Use 2-space indentation; continuation lines align beneath &
Use lower-case keywords and intrinsics (do, end subroutine, etc.)
Name modules with m_ pattern (e.g., m_transport)
Name public subroutines with s_ pattern (e.g., s_compute_flux)
Name public functions with f_ pattern
Keep subroutine size ≤ 500 lines, helper subroutines ≤ 150 lines, functions ≤ 100 lines, files ≤ 1000 lines
Limit routine arguments to ≤ 6; use derived-type params struct if more are needed
Forbid goto statements (except in legacy code), COMMON blocks, and save globals
Every argument must have explicit intent; use dimension/allocatable/pointer as appropriate
Call s_mpi_abort() for errors, never use stop or error stop
**/*.{fpp,f90}: Indent 2 spaces; continuation lines align under&
Use lower-case keywords and intrinsics (do,end subroutine, etc.)
Name modules withm_<feature>prefix (e.g.,m_transport)
Name public subroutines ass_<verb>_<noun>(e.g.,s_compute_flux) and functions asf_<verb>_<noun>
Keep private helpers in the module; avoid nested procedures
Enforce size limits: subroutine ≤ 500 lines, helper ≤ 150, function ≤ 100, module/file ≤ 1000
Limit subroutines to ≤ 6 arguments; otherwise pass a derived-type 'params' struct
Avoidgotostatements (except unavoidable legacy); avoid global state (COMMON,save)
Every variable must haveintent(in|out|inout)specification and appropriatedimension/allocatable/pointer
Uses_mpi_abort(<msg>)for error termination instead ofstop
Use!>style documentation for header comments; follow Doxygen Fortran format with!! @paramand!! @returntags
Useimplicit nonestatement in all modules
Useprivatedeclaration followed by explicitpublicexports in modules
Use derived types with pointers for encapsulation (e.g.,pointer, dimension(:,:,:) => null())
Usepureandelementalattributes for side-effect-free functions; combine them for array ...
Files:
src/common/m_model.fppsrc/common/m_variables_conversion.fppsrc/common/m_compute_levelset.fppsrc/simulation/m_time_steppers.fppsrc/simulation/m_data_output.fppsrc/simulation/m_derived_variables.fpp
src/**/*.fpp
📄 CodeRabbit inference engine (.cursor/rules/mfc-agent-rules.mdc)
src/**/*.fpp: Use.fppfile extension for Fypp preprocessed files; CMake transpiles them to.f90
Start module files with Fypp include for macros:#:include 'macros.fpp'
Use the fyppASSERTmacro for validating conditions:@:ASSERT(predicate, message)
Use fypp macro@:ALLOCATE(var1, var2)for device-aware allocation instead of standard Fortranallocate
Use fypp macro@:DEALLOCATE(var1, var2)for device-aware deallocation instead of standard Fortrandeallocate
Files:
src/common/m_model.fppsrc/common/m_variables_conversion.fppsrc/common/m_compute_levelset.fppsrc/simulation/m_time_steppers.fppsrc/simulation/m_data_output.fppsrc/simulation/m_derived_variables.fpp
src/simulation/**/*.{fpp,f90}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/simulation/**/*.{fpp,f90}: Wrap tight GPU loops with !$acc parallel loop gang vector default(present) reduction(...); add collapse(n) when safe; declare loop-local variables with private(...)
Allocate large GPU arrays with managed memory or move them into persistent !$acc enter data regions at start-up
Avoid stop/error stop inside GPU device code
Ensure GPU code compiles with Cray ftn, NVIDIA nvfortran, GNU gfortran, and Intel ifx/ifort compilers
src/simulation/**/*.{fpp,f90}: Mark GPU-callable helpers with$:GPU_ROUTINE(function_name='...', parallelism='[seq]')immediately after declaration
Do not use OpenACC or OpenMP directives directly; use Fypp macros fromsrc/common/include/parallel_macros.fppinstead
Wrap tight loops with$:GPU_PARALLEL_FOR(private='[...]', copy='[...]')macro; addcollapse=nfor safe nested loop merging
Declare loop-local variables withprivate='[...]'in GPU parallel loop macros
Allocate large arrays withmanagedor move them into a persistent$:GPU_ENTER_DATA(...)region at start-up
Do not placestoporerror stopinside device code
Files:
src/simulation/m_time_steppers.fppsrc/simulation/m_data_output.fppsrc/simulation/m_derived_variables.fpp
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.713Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Wrap tight GPU loops with !$acc parallel loop gang vector default(present) reduction(...); add collapse(n) when safe; declare loop-local variables with private(...)
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Mark GPU-callable helpers with `$:GPU_ROUTINE(function_name='...', parallelism='[seq]')` immediately after declaration
Applied to files:
src/common/m_variables_conversion.fpp
📚 Learning: 2025-11-24T21:50:16.713Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.713Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Wrap tight GPU loops with !$acc parallel loop gang vector default(present) reduction(...); add collapse(n) when safe; declare loop-local variables with private(...)
Applied to files:
src/common/m_variables_conversion.fpp
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Wrap tight loops with `$:GPU_PARALLEL_FOR(private='[...]', copy='[...]')` macro; add `collapse=n` for safe nested loop merging
Applied to files:
src/common/m_variables_conversion.fpp
📚 Learning: 2025-11-24T21:50:16.713Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.713Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Ensure GPU code compiles with Cray ftn, NVIDIA nvfortran, GNU gfortran, and Intel ifx/ifort compilers
Applied to files:
src/common/m_variables_conversion.fpptoolchain/dependencies/CMakeLists.txt
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Declare loop-local variables with `private='[...]'` in GPU parallel loop macros
Applied to files:
src/common/m_variables_conversion.fppsrc/simulation/m_derived_variables.fpp
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Do not use OpenACC or OpenMP directives directly; use Fypp macros from `src/common/include/parallel_macros.fpp` instead
Applied to files:
src/common/m_variables_conversion.fppsrc/simulation/m_derived_variables.fpp
📚 Learning: 2025-11-24T21:50:16.713Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.713Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Avoid stop/error stop inside GPU device code
Applied to files:
src/common/m_variables_conversion.fpp
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Compile with Cray `ftn` or NVIDIA `nvfortran` for GPU offloading; also build CPU-only with GNU `gfortran` and Intel `ifx`/`ifort` for portability
Applied to files:
toolchain/modulestoolchain/dependencies/CMakeLists.txt
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Applies to **/*.{fpp,f90} : Keep private helpers in the module; avoid nested procedures
Applied to files:
src/simulation/m_derived_variables.fpp
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Draft a step-by-step plan before making changes; build after each step using `./mfc.sh build -t pre_process simulation -j $(nproc)`
Applied to files:
toolchain/templates/tuo.mako
🧬 Code graph analysis (1)
toolchain/bootstrap/modules.sh (1)
toolchain/util.sh (2)
log(12-12)log_n(13-13)
🪛 Shellcheck (0.11.0)
toolchain/bootstrap/modules.sh
[warning] 49-49: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
[warning] 49-49: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: Oak Ridge | Frontier (CCE) (gpu-acc)
- GitHub Check: Code Cleanliness Check
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Coverage Test on CodeCov
- GitHub Check: Oak Ridge | Frontier (gpu-acc)
- GitHub Check: Github (ubuntu, mpi, no-debug, true)
- GitHub Check: Oak Ridge | Frontier (cpu)
- GitHub Check: Github (ubuntu, mpi, debug, false)
- GitHub Check: Github (ubuntu, mpi, debug, true)
- GitHub Check: Oak Ridge | Frontier (gpu-omp)
- GitHub Check: Github (macos, mpi, no-debug, false)
- GitHub Check: Github (macos, mpi, debug, false)
- GitHub Check: Github (ubuntu, no-mpi, single, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, no-debug, false)
- GitHub Check: Agent
- GitHub Check: Build & Publish
🔇 Additional comments (7)
toolchain/modules (1)
48-53: LGTM! Tuolumne module configuration is consistent with Frontier.The module setup for OLCF Tuolumne looks correct:
- ROCm 6.3.1 and CPE 25.03 align with Frontier's configuration
gfx942is the correct target for MI300A GPUsHSA_XNACK=0is appropriate for performanceNote: There's no
tuo-cpuentry defined. If CPU-only builds are intended to be supported on Tuolumne, you may need to add atuo-cpuline (similar to how Frontier hasf-gpubut also inherits fromf-all). If GPU-only is intentional, this is fine as-is.toolchain/bootstrap/modules.sh (1)
49-49: Shellcheck warning is a false positive for this color code pattern.The SC2027 warnings flag the adjacent quote concatenation (
$G""a$W), but this is an intentional pattern used throughout this file for embedding color codes. The pattern works correctly in bash to produce colored output.toolchain/templates/tuo.mako (1)
1-61: Template structure and Flux integration look reasonable.The overall template structure follows the pattern of other MFC templates with proper prologue/epilogue handling, conditional GPU support, and Flux scheduler integration appropriate for Tuolumne.
A few observations:
HSA_XNACK=0is set both intoolchain/modules(line 52) and here (line 39). The duplication is harmless but could be consolidated.- The
--coral2-hugepages=512GBdirective on line 14 is a large allocation—verify this is the intended configuration for Tuolumne workloads.toolchain/dependencies/CMakeLists.txt (1)
131-141: LGTM! HIPFORT version aligned with ROCm module.The update to
rocm-6.3.1correctly aligns the HIPFORT dependency with the ROCm version. This change is appropriately scoped to the Cray compiler path only (line 130).src/simulation/m_data_output.fpp (1)
1228-1239: Good fix usingE_idx—but elasticity branch likely needs the same correction.Switching the non-elastic call to
q_cons_vf(E_idx)%sf(...)matches theenergyargument ofs_compute_pressure. However, the elasticity path still passesq_cons_vf(1)%sf(...)asenergy, which looks inconsistent and likely wrong unless elasticity remaps indices. Please verify and align both branches ifenergyis always atE_idx.Likely follow-up fix (if indices are consistent under elasticity)
- call s_compute_pressure( & - q_cons_vf(1)%sf(j - 2, k, l), & + call s_compute_pressure( & + q_cons_vf(E_idx)%sf(j - 2, k, l), & q_cons_vf(alf_idx)%sf(j - 2, k, l), & dyn_p, pi_inf, gamma, rho, qv, rhoYks(:), pres, T, & q_cons_vf(stress_idx%beg)%sf(j - 2, k, l), & q_cons_vf(mom_idx%beg)%sf(j - 2, k, l), G_local)src/simulation/m_time_steppers.fpp (2)
49-50: LGTM!The module import follows the existing pattern in this file and is required for the new
s_compute_derived_variablescall.
536-539: Interface signature is compatible; call placement is correct.The call to
s_compute_derived_variables(t_step, q_cons_ts(1)%vf, q_prim_ts1, q_prim_ts2)matches the subroutine signature inm_derived_variables.fpp. All four arguments align with their corresponding parameters (integer fort_step, scalar_field array forq_cons_vf, and vector_field arrays forq_prim_ts1andq_prim_ts2). The placement within theprobe_wrtguard and afters_time_step_cyclingis appropriate.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 11 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="toolchain/templates/tuo.mako">
<violation number="1" location="toolchain/templates/tuo.mako:13">
P2: Missing space after `flux:` - inconsistent with all other flux directives in this file. This typo may cause the flux scheduler to not recognize this directive.</violation>
</file>
<file name="src/common/m_model.fpp">
<violation number="1" location="src/common/m_model.fpp:681">
P1: Division protection breaks for negative values. When `boundary_edge(2)` is negative, `max(sgm_eps, boundary_edge(2))` returns `sgm_eps`, causing incorrect sign and magnitude of `edgetan`. This will compute wrong normal directions. Consider using `sign(max(sgm_eps, abs(boundary_edge(2))), boundary_edge(2))` to preserve the sign while preventing division by zero.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Well, some of these AI suggestions seem valid, though the resulting code is somewhat gross. In particular, I'm referring to the preservation of the sign when preventing division by zero. I had to add these barriers because the run-time floating point exceptions were ocurring with debug builds on Tuo. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1103 +/- ##
==========================================
+ Coverage 44.00% 44.01% +0.01%
==========================================
Files 71 71
Lines 20293 20283 -10
Branches 1982 1979 -3
==========================================
- Hits 8929 8927 -2
+ Misses 10227 10219 -8
Partials 1137 1137 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
bot
added
size:M
|
CodeAnt AI Incremental review completed. |
1 participant
User description
User description
This PR adds support for Tuo with a modules entry and template. The following test cases fail still:
I do not have a timeline for resolving these issues, but the vast majority of cases work, and the setup should be good enough for small tests and such that don't use these features.
PR Type
Bug fix, Enhancement
Description
Fix division by zero errors in levelset and boundary calculations
Correct energy index reference in pressure computation
Add Tuolumne (Tuo) HPC system support with modules and template
Refactor derived variables computation to pass field arrays as parameters
Update hipfort dependency version to rocm-6.3.1
Diagram Walkthrough
File Walkthrough
3 files
Prevent division by zero in levelset normal calculationAdd epsilon protection to edge tangent calculationFix energy index reference in pressure computation6 files
Simplify GPU routine decorator parametersAdd field array parameters to derived variables subroutineRemove derived variables call and debug statementsAdd derived variables module and call in time stepping loopAdd Tuolumne system to module selection menuCreate Tuolumne job submission template1 files
Update hipfort rocm version to 6.3.11 files
Add Tuolumne module configuration and dependenciesCodeAnt-AI Description
Add Tuolumne (Tuolumne/tuo) execution support and fix a few runtime/accuracy issues
What Changed
Impact
✅ Run jobs on Tuolumne with batch/Flux✅ Fewer runtime NaNs and crashes from level-set/edge math✅ Correct probe and derived-variable outputs💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
Bug Fixes
New Features
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.