Skip to content

Output Immersed Boundary load and state data#1302

Draft
mrvandenboom wants to merge 13 commits intoMFlowCode:masterfrom
mrvandenboom:ibdata-output
Draft

Output Immersed Boundary load and state data#1302
mrvandenboom wants to merge 13 commits intoMFlowCode:masterfrom
mrvandenboom:ibdata-output

Conversation

@mrvandenboom
Copy link
Contributor

Description

This adds binary output of IB load and state data during the simulation run, and conversion to a CSV file in post processing. Including the parameter ib_state_wrt="T" will activate the output routines. Output files can be found under the "/D" folder.

Fixes #(issue)

Type of change

  • Bug fix
  • [ X] New feature
  • Refactor
  • Documentation
  • Other: describe

Testing

Ran lint, format, test -a on a MacBookPro (10 core) and on (4) Tuo GPUs.

Checklist

  • I added or updated tests for new behavior
  • [X ] I updated documentation if user-facing behavior changed

See the developer guide for full coding standards.

GPU changes (expand if you modified src/simulation/)
  • GPU results match CPU results
  • [ X] Tested on NVIDIA GPU or AMD GPU

Tests ran considerably faster on MacBook CPUs than on Tuolumne GPUs.

AI code reviews

Reviews are not triggered automatically. To request a review, comment on the PR:

  • @coderabbitai review — incremental review (new changes only)
  • /review — Qodo review
  • /improve — Qodo code suggestions

@github-actions
Copy link

Claude Code Review

Head SHA: 0c8d00c

Files changed: 12

  • src/simulation/m_data_output.fpp
  • src/simulation/m_time_steppers.fpp
  • src/simulation/m_global_parameters.fpp
  • src/simulation/m_start_up.fpp
  • src/simulation/m_mpi_proxy.fpp
  • src/post_process/m_data_output.fpp
  • src/post_process/m_global_parameters.fpp
  • src/post_process/m_start_up.fpp
  • src/post_process/m_mpi_proxy.fpp
  • src/post_process/p_main.fpp
  • toolchain/mfc/params/definitions.py
  • docs/documentation/case.md

Summary:

  • Adds ib_state_wrt parameter to write IB load/state data to a binary file during simulation and convert to per-IB CSV files in post-processing
  • The parameter is correctly registered in definitions.py, both targets' m_global_parameters.fpp, m_start_up.fpp namelists, and m_mpi_proxy.fpp broadcasts
  • Post-process side uses safe newunit= for file I/O; simulation side does not
  • Renames ib.datib_data.dat for the existing IB binary data file
  • No new tests added (checklist unchecked)

Findings

1. Hard-coded Fortran I/O unit —

Lines ~+267, +1160, +1947 (new s_open_ib_state_file, s_write_ib_state_file, s_close_ib_state_file)

The simulation-side routines use the literal unit number 92, while the post-process side (correctly) uses newunit=iu_in / newunit=iu_out(i). Hard-coded unit numbers risk conflicts with other open statements in the codebase. Use a module-level integer variable and newunit= instead:

! In module m_data_output (module-level)
integer :: ib_state_unit

! In s_open_ib_state_file
open(newunit=ib_state_unit, file=trim(file_loc), form='unformatted', access='stream', status='replace')

! In s_write_ib_state_file / s_close_ib_state_file
write(ib_state_unit) ...
close(ib_state_unit)

2. Contradictory STATUS='replace' + POSITION='append' — ~+267

open (92, FILE=trim(file_loc), &
      FORM='unformatted', &
      ACCESS='stream', &
      STATUS='replace', &    ! truncates/creates the file
      POSITION='append')     ! contradicts: positions at EOF

STATUS='replace' and POSITION='append' are semantically contradictory. For a freshly created file they happen to be harmless, but this is confusing and non-portable. Remove POSITION='append'.


3. Missing physics constraint in case_validator.py

Per the parameter-system rules: "Location 4 is required only if the parameter has physics constraints."

ib_state_wrt = .true. is meaningless (and may cause runtime errors accessing patch_ib data) unless ib = .true.. A validation rule should be added to toolchain/mfc/case_validator.py, e.g.:

if params.get('ib_state_wrt') and not params.get('ib'):
    raise MFCException('ib_state_wrt requires ib = .true.')

A corresponding Fortran-side @:PROHIBIT in m_checker.fpp or m_checker_common.fpp would also be appropriate.


4. Silent breaking change: ib.datib_data.dat — ~+1089

-        write (file_path, '(A,I0,A)') trim(t_step_dir)//'/ib.dat'
+        write (file_path, '(A,I0,A)') trim(t_step_dir)//'/ib_data.dat'

This renames the existing per-timestep IB binary data file without mention in the PR description. Any downstream script or tool reading ib.dat will silently fail. If this rename is intentional, it should be called out in the PR description and reflected in documentation. If it is not intentional (perhaps motivated by a name collision with the new ib_state.dat), confirm it is necessary.


5. No tests added

The PR checklist item "I added or updated tests for new behavior" is unchecked. Per CLAUDE.md: "YOU MUST run tests relevant to your changes before claiming work is done." Consider adding a test case that enables ib_state_wrt (e.g., based on an existing IB test) and verifies the output files are produced. See toolchain/mfc/test/cases.py for the CaseGeneratorStack pattern.


Minor / Nit

  • s_write_ib_state_file inconsistency: The open/close calls are guarded at the call site with proc_rank == 0, but the write call in m_time_steppers.fpp is called by all ranks (with the guard inside the subroutine). This works correctly, but is inconsistent with the surrounding pattern. Consider moving the proc_rank == 0 guard to the call site in m_time_steppers.fpp (lines ~635–643) for clarity.
  • character(LEN=...) capitalization: The simulation-side s_open_ib_state_file uses LEN= (uppercase) while the surrounding file uses lowercase len=. Minor style inconsistency.

Keep ib_state_wrt addition, apply ruff formatting from master.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions
Copy link

Claude Code Review

Head SHA: f3e1b45

Files changed: 12

  • src/simulation/m_data_output.fpp
  • src/simulation/m_time_steppers.fpp
  • src/simulation/m_global_parameters.fpp
  • src/simulation/m_start_up.fpp
  • src/simulation/m_mpi_proxy.fpp
  • src/post_process/m_data_output.fpp
  • src/post_process/m_global_parameters.fpp
  • src/post_process/m_start_up.fpp
  • src/post_process/m_mpi_proxy.fpp
  • src/post_process/p_main.fpp
  • toolchain/mfc/params/definitions.py
  • docs/documentation/case.md

Summary:

  • Adds ib_state_wrt parameter for binary output of IB force/torque/velocity/angle/centroid data during simulation, with post-processing conversion to per-IB CSV files
  • Parameter is correctly added to both simulation and post_process m_global_parameters.fpp, m_start_up.fpp, m_mpi_proxy.fpp, and definitions.py (4-location rule met for this param's scope)
  • Renames ib.datib_data.dat (breaking change for existing users)
  • No tests added (checklist unchecked)

Findings

1. Hard-coded Fortran unit number — line ~268

Hard-coding unit 92 risks silent collision with any other open using that unit number. Use the newunit= specifier instead:

Store iu_ib_state as a module-level variable so s_write_ib_state_file and s_close_ib_state_file can reference it.

2. Contradictory STATUS='replace' + POSITION='append' — line ~271

STATUS='replace' truncates/creates the file, making POSITION='append' a no-op. If the intent is to always start fresh, remove POSITION='append'. If the intent is to append across restarts, use STATUS='unknown' or STATUS='old'.

3. Error handling via print * + return — lines ~1520, ~1531

The codebase convention is to use @:PROHIBIT() or call s_mpi_abort() for fatal errors. A silent return here means post_process silently produces no output with no clear failure signal in batch jobs.

4. Missing constraint in case_validator.py — no file modified

ib_state_wrt = T without ib = T will silently do nothing (or worse, loop over uninitialized patch_ib entries). A case_validator.py entry enforcing ib_state_wrt → ib is missing. Per CLAUDE.md, location 4 is required when a parameter has physics/dependency constraints.

Consider also a Fortran-side check in m_checker.fpp:

5. s_compute_ib_forces called every RK stage for fixed IBs — lines ~638–642

Force computation is triggered at every time step for fixed IBs whenever ib_state_wrt is set, even if forces were already computed. Confirm this is not duplicating work or bypassing the normal control flow (force computation is normally gated by moving_immersed_boundary_flag).

6. Inconsistent rank-guard pattern —

s_open_ib_state_file and s_close_ib_state_file are called with an outer proc_rank == 0 guard, but s_write_ib_state_file is called from all ranks and has an internal guard. The inconsistency is harmless but confusing — apply the outer guard to the write call too, or remove the internal guard and always guard externally.

7. Breaking rename of ib.datib_data.dat — line ~1090

Any existing post-processing scripts or documentation referencing ib.dat will silently break. If this rename is intentional, it should be called out in the PR description and release notes.

8. No tests added

The PR checklist notes tests were not added. IB state output should have at least one regression test (a 2D IB case with ib_state_wrt = T) to prevent silent regressions in the binary record layout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants