add core changes to probe gen #1052
Conversation
|
CodeAnt AI is reviewing your PR. |
|
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. WalkthroughThe PR updates Silo-HDF5 offset option calls to include an explicit count parameter, adds precision-dependent numeric formatting and a NaN diagnostic in simulation output, introduces probe-specific parsing in the packer, and implements automatic probe placement and related defaults in test case generation. Changes
Sequence DiagramsequenceDiagram
participant Case as Test Case Generator<br/>(case.py)
participant Sim as Simulation Output<br/>(m_data_output.fpp)
participant Post as Post-process Writer<br/>(m_data_output.fpp)
participant Packer as Data Packer<br/>(pack.py)
Case->>Case: Ensure defaults (fd_order, probe_wrt)\nand auto-generate probe positions (2/4/8)
Case->>Sim: Provide probe configuration
Sim->>Sim: Select precision format (FMT_glb / FMT)
Sim->>Post: Write numeric output using dynamic formats\nand emit ICFL NaN diagnostic if needed
Post->>Packer: Produce probe data files
Packer->>Packer: If file path contains "probe", discard first column\nand parse remaining columns as doubles
Packer->>Case: Return/process packed probe data
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Points to inspect closely:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (10)
Tip 📝 Customizable high-level summaries are now available!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting. Comment |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
codeant-ai
bot
added
the
size:M
![qodo-merge-pro[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
There was a problem hiding this comment.
High-level Suggestion
The automatic probe placement at 1/3 and 2/3 of the domain is a generic heuristic that might miss key physical phenomena at boundaries or interfaces. Consider making this feature opt-in or providing easier override mechanisms to ensure probes monitor relevant regions. [High-level, importance: 8]
Solution Walkthrough:
Before:
# In toolchain/mfc/test/case.py
# ...
# This logic is applied to all test cases by default (opt-out)
if 'probe_wrt' not in case:
mods['probe_wrt'] = 'T'
if case_is_1D:
mods['num_probes'] = 2
# Place 2 probes at 1/3 and 2/3 of the domain
elif case_is_2D:
mods['num_probes'] = 4
# Place 4 probes in a 2x2 grid at 1/3 and 2/3 of domain
else: # 3D
mods['num_probes'] = 8
# Place 8 probes in a 2x2x2 grid at 1/3 and 2/3 of domainAfter:
# In toolchain/mfc/test/case.py
# ...
# Suggestion: Make this an opt-in feature
if case.get('auto_probes', False):
mods['probe_wrt'] = 'T'
if case_is_1D:
mods['num_probes'] = 2
# Place 2 probes at 1/3 and 2/3 of the domain
elif case_is_2D:
mods['num_probes'] = 4
# Place 4 probes in a 2x2 grid at 1/3 and 2/3 of domain
else: # 3D
mods['num_probes'] = 8
# Place 8 probes in a 2x2x2 grid at 1/3 and 2/3 of domain
# Individual tests would need to set `auto_probes: True` to enable this.
src/simulation/m_data_output.fpp
Outdated
| FMT = '(6X,F12.6,11'//FMT_glb//')' | ||
| write (i + 30, FMT) & | ||
| nondim_time, & | ||
| rho, & | ||
| vel(1), & | ||
| pres | ||
| print *, 'time =', nondim_time, 'rho =', rho, 'pres =', pres | ||
| vel(2), & | ||
| vel(3), & | ||
| pres, & ! Out of tolerance | ||
| gamma, & | ||
| pi_inf, & | ||
| qv, & | ||
| c, & | ||
| accel |
There was a problem hiding this comment.
✅ Suggestion: Correct the Fortran format string to use 10 instead of 11 for the number of floating-point fields to match the 11 variables being written. [possible issue, importance: 9]
| FMT = '(6X,F12.6,11'//FMT_glb//')' | |
| write (i + 30, FMT) & | |
| nondim_time, & | |
| rho, & | |
| vel(1), & | |
| pres | |
| print *, 'time =', nondim_time, 'rho =', rho, 'pres =', pres | |
| vel(2), & | |
| vel(3), & | |
| pres, & ! Out of tolerance | |
| gamma, & | |
| pi_inf, & | |
| qv, & | |
| c, & | |
| accel | |
| FMT = '(6X,F12.6,10'//FMT_glb//')' | |
| write (i + 30, FMT) & | |
| nondim_time, & | |
| rho, & | |
| vel(1), & | |
| vel(2), & | |
| vel(3), & | |
| pres, & ! Out of tolerance | |
| gamma, & | |
| pi_inf, & | |
| qv, & | |
| c, & | |
| accel |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/simulation/m_data_output.fpp
Outdated
| nondim_time, & | ||
| rho, & | ||
| vel(1), & | ||
| pres | ||
| print *, 'time =', nondim_time, 'rho =', rho, 'pres =', pres | ||
| vel(2), & | ||
| vel(3), & |
There was a problem hiding this comment.
When p == 0 the code falls into the 2‑D branch, where vel is declared with num_vels = num_dims = 2 (see m_global_parameters). The new write statement still dereferences vel(3) (and the elasticity block a few lines above does the same), so every 2‑D test that now has probes enabled will read past the end of the array, producing garbage or a bounds error as soon as probes are written. Only access vel(3) when num_vels ≥ 3.
Useful? React with 👍 / 👎.
src/simulation/m_data_output.fpp
Outdated
| FMT = '(6X,F12.6,11'//FMT_glb//')' | ||
| write (i + 30, FMT) & | ||
| nondim_time, & | ||
| rho, & | ||
| vel(1), & |
There was a problem hiding this comment.
The same p == 0 block now builds FMT = '(6X,F12.6,11'//FMT_glb//')', which requires eleven Fxx descriptors after the time column, yet the following write only passes ten scalars (ρ, u, v, w, p, γ, π∞, qv, c, accel). Fortran stops execution when the I/O list is shorter than the format, so every non‑bubble 2‑D case with probes enabled will abort while writing. Either lower the repeat count or provide the missing field.
Useful? React with 👍 / 👎.
src/simulation/m_data_output.fpp
Outdated
| FMT = '(6X,F12.6,11'//FMT_glb//')' | ||
| write (i + 30, FMT) & | ||
| nondim_time, & | ||
| rho, & | ||
| vel(1), & |
There was a problem hiding this comment.
The 3‑D branch now reuses the same '(6X,F12.6,11'//FMT_glb//')' format but only supplies eight scalars (ρ, the three velocities, p, γ, π∞ and qv). That causes the runtime to raise an I/O list length error, and even if the program survived, the sound speed and acceleration columns that existed before this commit are no longer emitted so probe goldens cannot check them. Restore the missing values (c and accel) or reduce the format width so the number of descriptors matches the output list.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull Request Overview
This pull request adds comprehensive probe file testing infrastructure to MFC's test suite. All tests now automatically generate probe output at strategic locations and validate against golden files. The PR includes changes to the test framework, packer utilities, and simulation/post-processing source code.
Key changes:
- Automatic probe placement based on dimensionality (2 probes for 1D, 4 for 2D, 8 for 3D)
- Enhanced probe file parsing in the packer to strip time columns for proper comparison
- Dynamic probe output formatting based on precision settings
- SILO database API bug fix for correct ghost zone offset handling
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| toolchain/mfc/test/case.py | Adds automatic probe configuration for all tests with strategic placement based on domain dimensionality (1D/2D/3D) |
| toolchain/mfc/packer/pack.py | Implements special parsing logic for probe files to strip time column and extract only physics values for golden file comparison |
| src/simulation/m_derived_variables.fpp | Code cleanup removing extraneous blank lines around probe file writing call |
| src/simulation/m_data_output.fpp | Adds dynamic probe output formatting based on precision, expands probe output variables, and adds NaN diagnostic print statement |
| src/post_process/m_data_output.fpp | Fixes SILO database API bug by changing from DBADDIOPT to DBADDIAOPT with proper array size parameters for ghost zone offsets |
| write (3, *) ! new line | ||
|
|
||
| if (.not. f_approx_equal(icfl_max_glb, icfl_max_glb)) then | ||
| print *, 'icfl', icfl_max_glb |
There was a problem hiding this comment.
[nitpick] The debug print statement was added to help diagnose NaN issues, which is helpful. However, note that there's already a print statement at line 369 for the ICFL > 1.0 case, so this maintains consistency with existing error diagnostics in this function. Consider whether this debug output should be retained in production code or removed after debugging is complete.
| print *, 'icfl', icfl_max_glb |
src/simulation/m_data_output.fpp
Outdated
| FMT = '(6X,F12.6,11'//FMT_glb//')' | ||
| write (i + 30, FMT) & | ||
| nondim_time, & | ||
| rho, & | ||
| vel(1), & | ||
| pres | ||
| print *, 'time =', nondim_time, 'rho =', rho, 'pres =', pres | ||
| vel(2), & | ||
| vel(3), & | ||
| pres, & ! Out of tolerance | ||
| gamma, & | ||
| pi_inf, & | ||
| qv, & | ||
| c, & | ||
| accel |
There was a problem hiding this comment.
Format string mismatch: The format specifies '(6X,F12.6,11'//FMT_glb//')' which expands to '(6X,F12.6,11F28.16)', expecting 12 total values (1 + 11), but the write statement outputs 11 values. The format should be '(6X,F12.6,10'//FMT_glb//')' to match the 10 values written after nondim_time.
Additionally, line 1610 has a leftover comment ! Out of tolerance that should be removed.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/simulation/m_data_output.fpp (1)
1133-1156: Format/argument mismatches and out-of-boundsvel(3)access confirmed—all three issues are critical runtime errorsThe review comment identifies valid correctness issues verified through code inspection:
2D elasticity (line 1591-1599): Accesses
vel(3)whennum_vels = 2in 2D simulations (confirmed:num_vels = num_dimsin non-MHD runs). This is an out-of-bounds array access; format count matches but the access is unsafe.2D non-elastic (line 1600-1615): Format string expects 11 values after
nondim_time, but only 10 are written (rho, vel(1:3), pres, gamma, pi_inf, qv, c, accel). This will cause a Fortran I/O runtime error. Also hasvel(3)out-of-bounds issue.3D non-elastic (line 1617-1625): Format string expects 11 values, but only 8 are written (
rho, vel(1:3), pres, gamma, pi_inf, qv). Runtime error; missingcandaccelcontradicts the stated intent for richer probe diagnostics.The proposed fix correctly addresses all three issues by adjusting format counts, removing
vel(3)from 2D branches, and addingc/accelto 3D output.
🧹 Nitpick comments (1)
toolchain/mfc/packer/pack.py (1)
139-142: Probe branch parsing looks correct; only minor polish possibleThe new
elif 'probe' in short_filepath:branch correctly discards the leading time column and flattens remaining values across lines, while safely skipping blank lines. The behavior matches the documented<time> <value1> <value2> ...format.If you want to trim a tiny bit of redundancy, you could drop the extra
float(e)and just reuse_extract_doubles’ output directly:- elif 'probe' in short_filepath: - # Probes are constructed as <time> <value1> <value2> ... - # Just discard the first column - doubles = [float(e) for line in content.strip().splitlines() if line.strip() for e in _extract_doubles(line)[1:]] + elif 'probe' in short_filepath: + ! Probes are constructed as <time> <value1> <value2> ... + ! Just discard the first column + doubles = [ + d + for line in content.strip().splitlines() if line.strip() + for d in _extract_doubles(line)[1:] + ]You may also consider tightening the
'probe'match to the filename portion if you ever add unrelated files whose paths contain that substring, but that’s optional for the current test harness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/post_process/m_data_output.fpp(2 hunks)src/simulation/m_data_output.fpp(5 hunks)src/simulation/m_derived_variables.fpp(0 hunks)toolchain/mfc/packer/pack.py(1 hunks)toolchain/mfc/test/case.py(1 hunks)
💤 Files with no reviewable changes (1)
- src/simulation/m_derived_variables.fpp
⏰ 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). (19)
- GitHub Check: Github (macos, mpi, debug, false)
- GitHub Check: Github (ubuntu, mpi, no-debug, false)
- GitHub Check: Github (macos, mpi, no-debug, false)
- GitHub Check: Github (ubuntu, no-mpi, single, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, no-debug, true)
- GitHub Check: Github (ubuntu, mpi, debug, true)
- GitHub Check: Github (ubuntu, mpi, debug, false)
- GitHub Check: Self Hosted (gpu, acc, gt)
- GitHub Check: Self Hosted (gpu, omp, frontier)
- GitHub Check: Self Hosted (gpu, acc, frontier)
- GitHub Check: Code Cleanliness Check
- GitHub Check: Georgia Tech | Phoenix (NVHPC) (gpu)
- GitHub Check: Georgia Tech | Phoenix (NVHPC) (gpu)
- GitHub Check: Oak Ridge | Frontier (CCE) (gpu)
- GitHub Check: Georgia Tech | Phoenix (NVHPC) (cpu)
- GitHub Check: Oak Ridge | Frontier (CCE) (gpu)
- GitHub Check: Coverage Test on CodeCov
- GitHub Check: Agent
- GitHub Check: Build & Publish
🔇 Additional comments (2)
src/post_process/m_data_output.fpp (1)
726-747: DBADDIAOPT usage and counts look consistent with offset dimensionsSwitching from
DBADDIOPTtoDBADDIAOPTwith explicit counts of 3 (3D) and 2 (2D) matches the allocated sizes oflo_offset/hi_offsetin those branches and should resolve the prior Silo API bug for ghost offsets.You might optionally also use
DBADDIAOPT(..., 1, lo_offset)in the 1D case for consistency, but the current change is already an improvement.Please double‑check against the Silo library version you build against that
DBADDIAOPT’s argument order is(optlist, opt, nvals, vals)and that 3/2 match the intended rank there.src/simulation/m_data_output.fpp (1)
365-371: Extra ICFL debug print is fine and scoped to rank 0The added
print *, 'icfl', icfl_max_glbin the NaN branch improves diagnostics and is already guarded byproc_rank == 0, so it won’t spam output from all ranks.No functional concerns here.
| mods['parallel_io'] = 'F' | ||
| mods['prim_vars_wrt'] = 'F' | ||
| if ('fd_order' not in case) and ('fd_order' not in mods): | ||
| mods['fd_order'] = 1 | ||
| if ('probe_wrt' not in case): | ||
| mods['probe_wrt'] = 'T' | ||
| if case['n'] == 0: | ||
| mods['num_probes'] = 2 | ||
| dx = (case['x_domain%end'] - case['x_domain%beg']) / 3 | ||
| for i in range(2): | ||
| mods[f'probe({{i+1}})%x'] = case['x_domain%beg'] + (i+1) * dx | ||
| elif case['p'] == 0: | ||
| mods['num_probes'] = 4 | ||
| dx = (case['x_domain%end'] - case['x_domain%beg']) / 3 | ||
| dy = (case['y_domain%end'] - case['y_domain%beg']) / 3 | ||
| for i in range(2): | ||
| for j in range(2): | ||
| mods[f'probe({{i*2+j+1}})%x'] = case['x_domain%beg'] + (i+1) * dx | ||
| mods[f'probe({{i*2+j+1}})%y'] = case['y_domain%beg'] + (j+1) * dy | ||
| else: | ||
| mods['num_probes'] = 8 | ||
| dx = (case['x_domain%end'] - case['x_domain%beg']) / 3 | ||
| dy = (case['y_domain%end'] - case['y_domain%beg']) / 3 | ||
| dz = (case['z_domain%end'] - case['z_domain%beg']) / 3 | ||
| for i in range(2): | ||
| for j in range(2): | ||
| for k in range(2): | ||
| mods[f'probe({{i*4+j*2+k+1}})%x'] = case['x_domain%beg'] + (i+1) * dx | ||
| mods[f'probe({{i*4+j*2+k+1}})%y'] = case['y_domain%beg'] + (j+1) * dy | ||
| mods[f'probe({{i*4+j*2+k+1}})%z'] = case['z_domain%beg'] + (k+1) * dz | ||
There was a problem hiding this comment.
🧩 Analysis chain
Auto probe defaults are reasonable but rely on domain keys being present
The new else-branch behavior does a few important things for tests:
- Forces serial IO (
parallel_io = 'F') and disables primitive-output (prim_vars_wrt = 'F') for non‑post_process runs. - Sets
fd_order = 1only if it isn’t already present in the base case or prior mods, which is a safe default for tests. - Enables probes by default (
probe_wrt = 'T'if missing) and places them at 1/3 and 2/3 of each domain extent (2 probes in 1D, 4 in 2D, 8 in 3D), giving consistent coverage across all test cases.
Two things to double‑check:
-
Domain-key assumptions: This logic assumes
casealways containsx_domain%beg/end(andy_domain%...,z_domain%...where needed). If any legacy or corner-case test lacks these keys, the generated case.py will crash when computingdx/dy/dz. You may want to either:- Add a small guard (e.g., fall back to a unit interval if keys are missing), or
- Explicitly document that all test cases must define these domain keys.
-
Default probe_wrt behavior: All non‑post_process runs will now emit probes unless the base case explicitly sets
probe_wrt. That’s consistent with the new probe testing infrastructure, but if any existing tests were relying on no probes, they should be updated to setprobe_wrt = 'F'explicitly.
Overall the structure is sound; just ensure the domain-key assumption holds across the full test suite.
I'll verify the concerns raised in the review comment by examining the context around the code snippet and checking for domain key initialization patterns.
---
🏁 Script executed:
```shell
#!/bin/bash
# Get broader context around lines 233-263 in case.py
cat toolchain/mfc/test/case.py | head -300 | tail -100
Length of output: 4017
🏁 Script executed:
#!/bin/bash
# Search for domain key initialization patterns in case.py
rg -n "x_domain|y_domain|z_domain" toolchain/mfc/test/case.py -B2 -A2 | head -50Length of output: 1453
🏁 Script executed:
#!/bin/bash
# Look for any guards or error handling around domain key access
rg -n "KeyError|try:|except|in case" toolchain/mfc/test/case.py | head -30Length of output: 103
🏁 Script executed:
#!/bin/bash
# Check the function signature and initial case dict population
rg -n "def |case\s*=" toolchain/mfc/test/case.py | head -20Length of output: 1037
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the gen_json_dict_str method definition
rg -n "def gen_json_dict_str|gen_json_dict_str" toolchain/mfc/test/case.py -B3 -A10 | head -40
Length of output: 526
🏁 Script executed:
#!/bin/bash
# Search for test case definitions and domain key initialization
rg -n "x_domain|domain" toolchain/mfc/test/ -l | head -10Length of output: 54
🏁 Script executed:
#!/bin/bash
# Look for where TestCase instances are created with mods
rg -n "define_case_f|mods\s*=" toolchain/mfc/test/ --type=py -B2 -A5 | head -60Length of output: 3759
🏁 Script executed:
#!/bin/bash
# Check if there are any test cases in the test directory
find toolchain/mfc/test -name "*.py" -type f | grep -v __pycache__Length of output: 112
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for TestCaseBuilder class and gen_json_dict_str method
rg -n "class TestCaseBuilder|def gen_json_dict_str" toolchain/mfc/test/case.py -A15 | head -80
Length of output: 523
🏁 Script executed:
#!/bin/bash
# Look for actual test case definitions with domain info in cases.py
rg -n "x_domain|y_domain|z_domain|define_case" toolchain/mfc/test/cases.py -B2 -A2 | head -100Length of output: 5227
🏁 Script executed:
#!/bin/bash
# Search for examples dir to see actual case.py files that might lack domain keys
find examples -name "case.py" -type f 2>/dev/null | head -5Length of output: 209
🏁 Script executed:
#!/bin/bash
# Examine one example case file to see domain key patterns
find examples -name "case.py" -type f 2>/dev/null | head -1 | xargs cat | head -80Length of output: 1605
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for gen_json_dict_str in the entire codebase
rg -n "gen_json_dict_str" toolchain/mfc/test/ -B2 -A8
Length of output: 700
🏁 Script executed:
#!/bin/bash
# Look for TestCase class definition and __init__ method
rg -n "class TestCase|def __init__" toolchain/mfc/test/case.py -A12 | head -60Length of output: 1534
🏁 Script executed:
#!/bin/bash
# Check a few more example case files to see if they all have domain keys
find examples -name "case.py" -type f 2>/dev/null | head -3 | xargs -I {} sh -c 'echo "=== {} ===" && grep -E "x_domain|y_domain|z_domain" {} || echo "NO DOMAIN KEYS"'Length of output: 351
🏁 Script executed:
#!/bin/bash
# Look at cases.py to see if there's any test that passes custom mods with domain keys
rg -n "x_domain|y_domain|z_domain" toolchain/mfc/test/cases.py -B3 -A3Length of output: 1979
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for BASE_CFG definition
rg -n "BASE_CFG" toolchain/mfc/test/case.py -B5 -A10
Length of output: 1307
🏁 Script executed:
#!/bin/bash
# Look for case.Case import and find where it's defined
rg -n "^import|^from.*case|class Case" toolchain/mfc/test/case.py | head -20Length of output: 181
🏁 Script executed:
#!/bin/bash
# Search for BASE_CFG and gen_json_dict_str in the input module (likely case.Case parent)
find toolchain -name "*.py" -type f | xargs rg -l "BASE_CFG|gen_json_dict_str" | head -5Length of output: 48
🏁 Script executed:
#!/bin/bash
# Check if any test case in cases.py is defined without domain keys
rg -n "define_case_d|define_case_f" toolchain/mfc/test/cases.py -A3 | grep -v "x_domain\|y_domain\|z_domain" | head -50Length of output: 1874
---
🏁 Script executed:
```shell
#!/bin/bash
# Get full BASE_CFG definition
rg -n "BASE_CFG = {" toolchain/mfc/test/case.py -A100 | head -120
Length of output: 87
🏁 Script executed:
#!/bin/bash
# Search for gen_json_dict_str in parent case.Case
rg -n "def gen_json_dict_str" toolchain/mfc/case.py -B3 -A15Length of output: 631
🏁 Script executed:
#!/bin/bash
# Check for any test that might trigger probe placement without domain keys
# Look for cases with n=0 or p=0 and empty or minimal mods
rg -n "define_case_d.*n.*0|define_case_f" toolchain/mfc/test/cases.py -B2 -A5 | head -80Length of output: 4077
🏁 Script executed:
#!/bin/bash
# Search for any get_parameters or similar that might show what goes into case dict
rg -n "get_parameters|params" toolchain/mfc/test/case.py -B2 -A5 | head -40Length of output: 1842
---
🏁 Script executed:
```shell
#!/bin/bash
# Get BASE_CFG using cat instead of rg regex
sed -n '14,150p' toolchain/mfc/test/case.py
Length of output: 5523
🏁 Script executed:
#!/bin/bash
# Search for x_domain in BASE_CFG section
rg -n "x_domain" toolchain/mfc/test/case.py -B3 -A3Length of output: 1447
🏁 Script executed:
#!/bin/bash
# Check TestCaseBuilder.to_case() to see how it loads params
rg -n "def to_case" toolchain/mfc/test/case.py -A30Length of output: 1038
🏁 Script executed:
#!/bin/bash
# Look for test cases that might have n=0 or p=0 (1D/2D) without explicit domain mods
rg -n "stack.push.*1D|n.*:.*0|p.*:.*0" toolchain/mfc/test/cases.py -B2 -A8 | head -100Length of output: 6890
Add guards for missing domain keys or document requirement
The code at lines 241, 246–256, 260–262 accesses domain keys (x_domain%beg/end, y_domain%beg/end, z_domain%beg/end) without validation. While most test cases populate these keys via stack setup, BASE_CFG lacks domain keys. If a test case is created without proper domain initialization (e.g., via custom define_case_f with a path or mods that omit domain keys), accessing these keys will raise KeyError at runtime.
Recommendations:
- Add defensive checks before accessing domain keys (use
.get()with fallback values or explicit key validation), or - Explicitly document that all test case definitions must include domain keys for their respective dimensions (1D→x_domain, 2D→x/y_domain, 3D→x/y/z_domain), and validate this invariant early in case setup.
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
src/simulation/m_data_output.fpp
Outdated
| qv, & | ||
| c, & | ||
| accel | ||
| qv |
There was a problem hiding this comment.
✅ Suggestion: Append c and accel to the write argument list so the FMT format that expects eleven values after rho receives the sound-speed and acceleration diagnostics it used to emit and avoids a runtime format mismatch. [possible bug]
|
CodeAnt AI finished reviewing your PR. |
1 participant
User description
User description
Overview
Toolchain Changes (
toolchain/)toolchain/mfc/packer/pack.py<time> <value1> <value2> ...toolchain/mfc/test/case.pyprobe_wrt = 'T') for all tests unless explicitly disabledfd_order = 1as default for all teststoolchain/mfc/packer/tol.py(already present)_format_error_diagnostics()function shows:find_maximum_errors_among_failing()function scans all files to identify worst errorsSource Code Changes (
src/)src/post_process/m_data_output.fppDBADDIOPT()→DBADDIAOPT()for grid offset optionssrc/simulation/m_data_output.fppFMT_glbcharacter variable controls output precisionprecisionparameter:F28.7F28.16printstatement for ICFL NaN detectionsrc/simulation/m_derived_variables.fpps_write_probe_files()callImpact & Benefits
Technical Details
PR Type
Bug fix, Enhancement
Description
Fixed SILO database API bug: changed
DBADDIOPT()toDBADDIAOPT()with proper array size parametersImplemented dynamic probe output formatting based on precision parameter (F28.7 for single, F28.16 for double)
Added automatic probe placement for all test cases based on dimensionality (2 for 1D, 4 for 2D, 8 for 3D)
Enhanced probe file parsing in packer to strip time column and extract values only
Added ICFL NaN detection debug output and improved probe output variable coverage
Diagram Walkthrough
File Walkthrough
m_data_output.fpp
Fix SILO API grid offset option callssrc/post_process/m_data_output.fpp
DBADDIOPT()withDBADDIAOPT()for grid offset options
option calls
m_data_output.fpp
Implement dynamic probe output formattingsrc/simulation/m_data_output.fpp
FMT_glbvariable based onprecision parameter
F28.16
components
and accel variables
pack.py
Add probe file time column strippingtoolchain/mfc/packer/pack.py
(first column)
...now extracts only valuescase.py
Implement automatic probe configurationtoolchain/mfc/test/case.py
probe_wrt = 'T') for all testsunless explicitly disabled
- 1D
cases: 2 probes at 1/3 and 2/3 of domain
- 2D cases: 4 probes in 2×2
grid
- 3D cases: 8 probes in 2×2×2 grid
fd_order = 1as default when not explicitly specifiedm_derived_variables.fpp
Clean up whitespace around probe file callssrc/simulation/m_derived_variables.fpp
s_write_probe_files()ands_write_com_files()callsCodeAnt-AI Description
Standardize probe output and correct SILO ghost offsets
What Changed
Impact
✅ Accurate SILO ghost zones✅ Stable probe regression data✅ Better probe diagnostics💡 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
New Features
Bug Fixes