Skip to content

add core changes to probe gen #1052

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
+54 −14
Draft

add core changes to probe gen #1052

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
929a2fc
add core changes to probe gen
sbryngelson Nov 17, 2025
30e32d6
fix
sbryngelson Nov 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/post_process/m_data_output.fpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -725,8 +725,8 @@ contains

if (p > 0) then
err = DBMKOPTLIST(2, optlist)
err = DBADDIOPT(optlist, DBOPT_LO_OFFSET, lo_offset)
err = DBADDIOPT(optlist, DBOPT_HI_OFFSET, hi_offset)
err = DBADDIAOPT(optlist, DBOPT_LO_OFFSET, 3, lo_offset)
err = DBADDIAOPT(optlist, DBOPT_HI_OFFSET, 3, hi_offset)
if (grid_geometry == 3) then
err = DBPUTQM(dbfile, 'rectilinear_grid', 16, &
'x', 1, 'y', 1, 'z', 1, &
Expand All @@ -743,8 +743,8 @@ contains
err = DBFREEOPTLIST(optlist)
elseif (n > 0) then
err = DBMKOPTLIST(2, optlist)
err = DBADDIOPT(optlist, DBOPT_LO_OFFSET, lo_offset)
err = DBADDIOPT(optlist, DBOPT_HI_OFFSET, hi_offset)
err = DBADDIAOPT(optlist, DBOPT_LO_OFFSET, 2, lo_offset)
err = DBADDIAOPT(optlist, DBOPT_HI_OFFSET, 2, hi_offset)
err = DBPUTQM(dbfile, 'rectilinear_grid', 16, &
'x', 1, 'y', 1, 'z', 1, &
x_cb, y_cb, DB_F77NULL, dims, 2, &
Expand Down
24 changes: 17 additions & 7 deletions src/simulation/m_data_output.fpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,7 @@ contains
write (3, *) ! new line

if (.not. f_approx_equal(icfl_max_glb, icfl_max_glb)) then
print *, 'icfl', icfl_max_glb
Copy link

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Suggested change
print *, 'icfl', icfl_max_glb

Copilot uses AI. Check for mistakes.
call s_mpi_abort('ICFL is NaN. Exiting.')
elseif (icfl_max_glb > 1._wp) then
print *, 'icfl', icfl_max_glb
Expand Down Expand Up @@ -1129,6 +1130,8 @@ contains
real(wp) :: G_local
real(wp) :: dyn_p, T
real(wp) :: damage_state
character(LEN=15) :: FMT_glb
character(len=30) :: FMT

integer :: i, j, k, l, s, d !< Generic loop iterator

Expand All @@ -1145,6 +1148,13 @@ contains

T = dflt_T_guess

! Set format string based on precision
if (precision == 1) then
FMT_glb = 'F28.7'
else
FMT_glb = 'F28.16'
end if

! Non-dimensional time calculation
if (time_stepper == 23) then
nondim_time = mytime
Expand Down Expand Up @@ -1578,8 +1588,8 @@ contains
R(1), &
Rdot(1)
else if (elasticity) then
write (i + 30, '(6X,F12.6,F24.8,F24.8,F24.8,F24.8,'// &
'F24.8,F24.8,F24.8)') &
FMT = '(6X,F12.6,7'//FMT_glb//')'
write (i + 30, FMT) &
nondim_time, &
rho, &
vel(1), &
Expand All @@ -1589,17 +1599,17 @@ contains
tau_e(2), &
tau_e(3)
else
write (i + 30, '(6X,F12.6,F24.8,F24.8,F24.8)') &
FMT = '(6X,F12.6,4'//FMT_glb//')'
write (i + 30, FMT) &
nondim_time, &
rho, &
vel(1), &
vel(2), &
pres
print *, 'time =', nondim_time, 'rho =', rho, 'pres =', pres
end if
else
write (i + 30, '(6X,F12.6,F24.8,F24.8,F24.8,F24.8,'// &
'F24.8,F24.8,F24.8,F24.8,F24.8,'// &
'F24.8)') &
FMT = '(6X,F12.6,10'//FMT_glb//')'
write (i + 30, FMT) &
nondim_time, &
rho, &
vel(1), &
Expand Down
2 changes: 0 additions & 2 deletions src/simulation/m_derived_variables.fpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,9 +168,7 @@ contains
$:GPU_UPDATE(host='[accel_mag]')

call s_derive_center_of_mass(q_prim_ts2(2)%vf, c_mass)

call s_write_probe_files(t_step, q_cons_ts(1)%vf, accel_mag)

call s_write_com_files(t_step, c_mass)
end if

Expand Down
4 changes: 4 additions & 0 deletions toolchain/mfc/packer/pack.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,10 @@ def _extract_doubles(s: str) -> list:
lines = content.splitlines()
content = "\n".join(lines[1:]) # Skip the first line
doubles = _extract_doubles(content)
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:]]
else:
# Every line is <x> <y> <z> <value> (<y> and <z> are optional). So the
# number of dimensions is the number of doubles in the first line minus 1.
Expand Down
30 changes: 29 additions & 1 deletion toolchain/mfc/test/case.py
View file
Open in desktop
Copy link
Contributor

@qodo-merge-pro qodo-merge-pro bot Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 domain

After:

# 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.

Original file line number Diff line number Diff line change
Expand Up @@ -230,8 +230,36 @@ def create_directory(self):
mods['omega_wrt(2)'] = 'T'
mods['omega_wrt(3)'] = 'T'
else:
mods['parallel_io'] = 'F'
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

Comment on lines +233 to 263
Copy link

@coderabbitai coderabbitai bot Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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 = 1 only 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:

  1. Domain-key assumptions: This logic assumes case always contains x_domain%beg/end (and y_domain%..., z_domain%... where needed). If any legacy or corner-case test lacks these keys, the generated case.py will crash when computing dx/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.

  2. 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 set probe_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 -50

Length 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 -30

Length 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 -20

Length 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 -10

Length 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 -60

Length 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 -100

Length 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 -5

Length 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 -80

Length 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 -60

Length 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 -A3

Length 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 -20

Length 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 -5

Length 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 -50

Length 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 -A15

Length 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 -80

Length 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 -40

Length 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 -A3

Length 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 -A30

Length 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 -100

Length 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:

  1. Add defensive checks before accessing domain keys (use .get() with fallback values or explicit key validation), or
  2. 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.

print(json.dumps({{**case, **mods}}))
""")
Expand Down
Loading