Add CLI visualization command (./mfc.sh viz)#1233
Add CLI visualization command (./mfc.sh viz)#1233sbryngelson merged 105 commits intoMFlowCode:masterfrom
Conversation
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (9)
toolchain/pyproject.toml (1)
40-43: Pin version lower-bounds and consider makingimageio-ffmpegan optional extra.Two actionable concerns:
No version constraints — both
imageioandimageio-ffmpegare unpinned. The latestimageiois 2.37.2 and the latestimageio-ffmpegis 0.6.0 (released Jan 16, 2025). Add at minimum a lower-bound to prevent silent regressions if either library releases breaking changes.Mandatory ~60 MiB binary —
imageio-ffmpeg's platform-specific wheels contain the ffmpeg executable, making the package around 60 MiB in size. Sinceimageio-ffmpegis only needed for--mp4video generation and not for PNG output, making it an optional extra avoids this cost for all other toolchain users.♻️ Suggested approach
- # Visualization (video rendering) - "imageio", - "imageio-ffmpeg", -Keep
imageioas a required dep (lightweight) and moveimageio-ffmpegto an optional extra:dependencies = [ ... # Visualization (image I/O) "imageio>=2.33", ... ] [project.optional-dependencies] viz-video = [ "imageio-ffmpeg>=0.5.0", ]Then document that
pip install mfc[viz-video](or./mfc.shcan detect and prompt on first--mp4use).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/pyproject.toml` around lines 40 - 43, Update pyproject.toml to pin lower bounds for the image I/O packages and move the heavy ffmpeg wheel into an optional extra: add a minimum version constraint for "imageio" in the dependencies list (e.g., "imageio>=2.33") and remove "imageio-ffmpeg" from the main dependencies, then add an [project.optional-dependencies] (or equivalent) section named something like "viz-video" containing "imageio-ffmpeg>=0.5.0" so users can opt into the ~60 MiB binary only when they need MP4 generation.toolchain/mfc/viz/viz.py (1)
14-34: Single-int step is not validated against available timesteps, unlike ranges.When
step_argis a range (e.g.,"0:10000:500"), the result is filtered againstavailable_steps. But a single int (line 34) is returned as-is without checking membership. This means--step 999will proceed even if step 999 doesn't exist, deferring failure to the reader (likely with an opaque file-not-found error).Consider filtering the single-int case as well for a consistent UX:
Proposed fix
- return [int(step_arg)] + single = int(step_arg) + return [single] if single in set(available_steps) else []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/viz/viz.py` around lines 14 - 34, The single-int branch in _parse_steps doesn't validate step_arg against available_steps; update the final return path to convert step_arg to int, check membership against available_steps (or its set) and return [that_int] only if present, otherwise return an empty list (matching the range filtering behavior) so callers get consistent, pre-validated timesteps.toolchain/mfc/viz/reader.py (5)
264-368: Assembly logic is heavily duplicated withassemble_siloinsilo_reader.py.The multi-processor assembly logic (building global coordinate arrays with
np.unique/np.round, initializing global variable arrays byndim, and placing data viasearchsorted/np.ix_) is nearly identical betweenassemble()(lines 314–368) andassemble_silo()(silo_reader.py lines 204–269). Consider extracting a shared_assemble_from_proc_data(proc_data, ndim)helper inreader.pyand calling it from both assemblers. This would keep the format-specific I/O separate while unifying the coordinate-merge and data-placement math.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/viz/reader.py` around lines 264 - 368, The multi-processor assembly logic in assemble() duplicates assemble_silo(); extract the shared coordinate-merge and data-placement code into a new helper (suggest name _assemble_from_proc_data(proc_data, ndim)) that returns (ndim, global_x, global_y, global_z, global_vars) given proc_data and use it from assemble() and from assemble_silo() in silo_reader.py so file I/O stays format-specific while the unique math (np.unique/np.round, initializing arrays by ndim, searchsorted + np.ix_ placement) is centralized; update assemble() to call _assemble_from_proc_data(proc_data, ndim) after building proc_data and remove the duplicated assembly block.
116-133: Grid precision auto-detection relies on floating-point division — this is fine but could be simplified.The approach of dividing
grid_bytes / n_valsand checking proximity to 4.0 or 8.0 works becausen_valsmust evenly dividegrid_bytes. You could use integer modulo for a more robust check.♻️ Suggested simplification using integer arithmetic
- # Auto-detect grid precision from record size - bytes_per_val = grid_bytes / n_vals - if abs(bytes_per_val - 8.0) < 0.5: - grid_dtype = np.dtype(f'{endian}f8') - elif abs(bytes_per_val - 4.0) < 0.5: - grid_dtype = np.dtype(f'{endian}f4') + # Auto-detect grid precision from record size + if grid_bytes == n_vals * 8: + grid_dtype = np.dtype(f'{endian}f8') + elif grid_bytes == n_vals * 4: + grid_dtype = np.dtype(f'{endian}f4')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/viz/reader.py` around lines 116 - 133, The current auto-detection computes bytes_per_val = grid_bytes / n_vals and compares to 8.0/4.0 using floats; replace this with integer arithmetic to avoid FP rounding: compute n_vals (as already done) and use integer modulo/division on grid_bytes (e.g., grid_bytes % n_vals and grid_bytes // n_vals) to verify grid_bytes divides evenly and that grid_bytes // n_vals equals 8 or 4, then set grid_dtype accordingly (use the same endian f-strings); if it doesn't divide evenly or the quotient is not 8 or 4, raise the same ValueError with the existing message.
200-240:discover_timestepsduplicates silo timestep logic already insilo_reader.discover_timesteps_silo.Lines 228–238 duplicate the same silo timestep discovery that exists in
silo_reader.py:discover_timesteps_silo. Whenfmt == 'silo', this function could delegate todiscover_timesteps_siloto avoid maintaining two copies. Alternatively, centralize all format-aware discovery here and remove the standalone silo version.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/viz/reader.py` around lines 200 - 240, The silo-specific timestep discovery block in discover_timesteps duplicates logic in silo_reader.discover_timesteps_silo; replace the entire fmt == 'silo' branch with a call/delegation to silo_reader.discover_timesteps_silo(case_dir) (and add an import for silo_reader if missing) and return its result, removing the duplicated code so only discover_timesteps and discover_timesteps_silo remain as single sources of truth.
296-302: Silently skipping missing or empty processor files may hide data issues.If a processor file is missing (line 298:
continue) or has all-zero dimensions (lines 300–301), the assembly proceeds without warning. For a visualization tool this is a pragmatic choice, but a logged warning would help users debug incomplete post-processing runs.♻️ Add a warning for skipped processors
+import warnings ... if not os.path.isfile(fpath): + warnings.warn(f"Processor file not found, skipping: {fpath}") continue pdata = read_binary_file(fpath, var_filter=var) if pdata.m == 0 and pdata.n == 0 and pdata.p == 0: + warnings.warn(f"Processor p{rank} has zero dimensions, skipping") continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/viz/reader.py` around lines 296 - 302, The loop that builds proc_data silently skips missing files and zero-dimension reads; update the block around fpath/read_binary_file so it emits a warning whenever a processor is skipped: when os.path.isfile(fpath) is false log a warning including rank, fpath, step and var; after calling read_binary_file, if pdata.m == 0 and pdata.n == 0 and pdata.p == 0 log a warning including rank, fpath and the empty dimensions before continuing; use the module's logger or the logging.warning API to provide clear, contextual messages referencing fpath, rank, step, var and pdata so users can diagnose missing/empty processor files.
47-62:read_recordis unused dead code; remove it or make it private.This function is defined but never called anywhere in the codebase or module exports. It also has two legitimate issues worth fixing if retained:
Endianness handling is unreliable: It tries little-endian first, falls back to big-endian, but doesn't track which succeeded. The calling code has no way to know the payload's byte order.
Trailing record marker is never validated: Both
read_recordand_read_record_endianconsume the trailing 4-byte marker without verifying it matches the leading marker. This misses Fortran format violations that indicate file corruption.The codebase consistently uses
_read_record_endian(with detected endianness), so either removeread_recordor clearly mark it as private (_read_record) with a comment explaining its purpose if there's a reason to keep it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/viz/reader.py` around lines 47 - 62, The free-standing read_record function is unused and should either be removed or renamed to _read_record with a comment explaining it's a convenience wrapper only used for ad-hoc reads; if you keep it, change its behavior to detect and return endianness (or delegate to the existing _read_record_endian) and validate the trailing 4-byte marker matches the leading marker before returning the payload to avoid silent corruption; update references to use _read_record or remove the function entirely and ensure no other code relies on read_record.toolchain/mfc/viz/silo_reader.py (2)
126-132: The Fortran-order reinterpretation is correct but fragile — consider a clarifying note.The
ravel().reshape(data.shape, order='F')trick works because Silo/HDF5 preserves the Fortran dimension ordering(nx, ny, nz)as the dataset shape, so the reinterpretation produces the desireddata[ix, iy, iz]indexing. If the HDF5 shape ever changes (e.g., a Silo version reverses dims), this would silently produce transposed results. A brief inline note clarifying this assumption would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/viz/silo_reader.py` around lines 126 - 132, Add a brief inline comment next to the Fortran-order reinterpretation in silo_reader.py (the block that does data = np.ascontiguousarray(data).ravel().reshape(data.shape, order='F') and assigns into variables[key]) stating the assumption that the HDF5/Silo dataset shape preserves the Fortran dimension ordering (nx, ny, nz), and note that if that ordering ever changes the reshape will silently transpose data; optionally add a runtime sanity check (shape or known axis order) or a warning log to detect unexpected dimension-order changes before assigning to variables[key].
42-49: Remove_read_silo_objector refactorread_silo_fileto use it.This helper function is never called.
read_silo_filedirectly accessesobj.attrs["silo"](lines 85, 122) after checking the type inline, duplicating the logic in_read_silo_object.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/viz/silo_reader.py` around lines 42 - 49, The helper _read_silo_object is dead code or duplicated logic; either remove it or update read_silo_file to call it instead of repeating type and attribute checks. Locate the two places in read_silo_file where the code checks isinstance(obj, h5py.Datatype) and accesses obj.attrs["silo"] (currently duplicated) and replace that block with a call to _read_silo_object(obj) and handle a None return (skip/raise) the same way the inline code currently does; alternatively, delete the unused _read_silo_object function if you prefer to keep the inline checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/documentation/visualization.md`:
- Line 12: In the visualization description string that currently reads "MFC
includes a built-in visualization command that renders PNG images and MP4 videos
directly from post-processed output — no external GUI tools needed.", replace
the redundant phrase "PNG images" with "PNGs" so the sentence reads "...renders
PNGs and MP4 videos..."; update the sentence text in the visualization.md
content accordingly to match the `.typos.toml` convention.
- Line 12: Replace the phrase "PNG images" in the sentence that begins "MFC
includes a built-in visualization command that renders PNG images and MP4
videos..." with "PNGs" so the sentence reads "...renders PNGs and MP4 videos..."
(this aligns with the .typos.toml change and removes the redundancy).
In `@toolchain/mfc/viz/renderer.py`:
- Around line 203-221: The MP4 writer code should ensure writer is always closed
and surface a clear message when imageio is missing: wrap imageio.get_writer and
the for-loop that calls writer.append_data in a with-context or try/finally
around the writer object (reference the local variable writer and the
append_data calls) so writer.close() runs even if append_data raises, and change
the ImportError handler (currently swallowing the exception) to log or print a
clear error that imageio is not installed and set success=False; also ensure
other exceptions caught in the broad except log the error and keep success False
so the caller knows the MP4 was not produced.
In `@toolchain/mfc/viz/silo_reader.py`:
- Around line 82-85: The code reads obj.attrs["silo"] without guaranteeing the
"silo" attribute exists after checking silo_type; update both places (the block
that sets mesh_name/mesh_attr and the later block around line 122) to first
fetch the attribute defensively (e.g., use obj.attrs.get("silo") or test '"silo"
in obj.attrs') and handle the missing case by skipping the object or logging and
continuing; specifically modify the logic that checks silo_type and sets
mesh_attr to verify the "silo" attribute exists before assigning mesh_attr and
avoid raising KeyError.
- Around line 102-105: ProcessorData.m/n/p currently use a different convention
in silo_reader.py (m = len(x_cb)-1) than the binary reader; normalize to the
binary/Fortran convention and document it: set m = len(x_cb) - 2 (and similarly
n = len(y_cb) - 2, p = len(z_cb) - 2 when ndims >= 2/3) so ProcessorData.m
matches the Fortran-style m where m+1 = number of cells and x_cb has m+2
elements, and add/update the ProcessorData docstring/comment to state this
semantic explicitly.
In `@toolchain/mfc/viz/viz.py`:
- Around line 180-188: The MP4 generation branch calls render_mp4(varname,
requested_steps, mp4_path, ...) but silently returns when render_mp4 returns
False; update the block that checks the return value of render_mp4 to log an
error or warning via cons.print (include context like mp4_path and a hint about
missing dependencies such as imageio) when success is False, so the user is
informed that MP4 was not created; keep the existing success path unchanged and
ensure ARG('mp4'), render_mp4, fps, mp4_path, and read_step are used to form the
diagnostic message.
In `@toolchain/pyproject.toml`:
- Around line 40-43: Add lower-bound pins for the visualization deps and move
them to optional extras: replace the bare "imageio" and "imageio-ffmpeg" entries
with version-bounded requirements (e.g., imageio>=2.33, imageio-ffmpeg>=0.4.9)
and declare them under project.optional-dependencies (e.g., a "viz" extra list)
so users only install the large imageio-ffmpeg binary when they opt into the viz
extra; update any docs or installer scripts to reference pip install mfc[viz] or
to lazily install the "viz" extra when running the viz-related command.
---
Duplicate comments:
In `@toolchain/mfc/viz/silo_reader.py`:
- Around line 139-151: The function discover_timesteps_silo duplicates logic
already present in discover_timesteps(case_dir, fmt='silo') in reader.py; remove
the duplicate by replacing usages of discover_timesteps_silo with a call to
discover_timesteps(case_dir, fmt='silo') or move the implementation into a
shared helper and call that from both modules (update imports and references
accordingly). Ensure you keep the same behavior (filtering p0/*.silo, ignoring
"collection" files and non-integer names) and update any tests/imports to
reference discover_timesteps or the new shared helper instead of
discover_timesteps_silo.
- Around line 204-269: The multi-processor assembly block in silo_reader.py (the
loop building proc_centers, global_x/global_y/global_z, global_vars population
and final AssembledData return) duplicates logic from reader.py's
reader.assemble; extract that logic into a shared helper (e.g.,
assemble_proc_data or assemble_multi_processor) that accepts proc_data and
returns an AssembledData, then replace the duplicated block in silo_reader.py
with a call to that helper; ensure the helper handles ndim detection from
proc_data, uses unique rounding/np.searchsorted semantics, preserves variable
names (variables, x_cc/y_cc/z_cc, nx/ny/nz) and returns AssembledData so callers
(silo_reader.assemble and reader.assemble) can both call it.
---
Nitpick comments:
In `@toolchain/mfc/viz/reader.py`:
- Around line 264-368: The multi-processor assembly logic in assemble()
duplicates assemble_silo(); extract the shared coordinate-merge and
data-placement code into a new helper (suggest name
_assemble_from_proc_data(proc_data, ndim)) that returns (ndim, global_x,
global_y, global_z, global_vars) given proc_data and use it from assemble() and
from assemble_silo() in silo_reader.py so file I/O stays format-specific while
the unique math (np.unique/np.round, initializing arrays by ndim, searchsorted +
np.ix_ placement) is centralized; update assemble() to call
_assemble_from_proc_data(proc_data, ndim) after building proc_data and remove
the duplicated assembly block.
- Around line 116-133: The current auto-detection computes bytes_per_val =
grid_bytes / n_vals and compares to 8.0/4.0 using floats; replace this with
integer arithmetic to avoid FP rounding: compute n_vals (as already done) and
use integer modulo/division on grid_bytes (e.g., grid_bytes % n_vals and
grid_bytes // n_vals) to verify grid_bytes divides evenly and that grid_bytes //
n_vals equals 8 or 4, then set grid_dtype accordingly (use the same endian
f-strings); if it doesn't divide evenly or the quotient is not 8 or 4, raise the
same ValueError with the existing message.
- Around line 200-240: The silo-specific timestep discovery block in
discover_timesteps duplicates logic in silo_reader.discover_timesteps_silo;
replace the entire fmt == 'silo' branch with a call/delegation to
silo_reader.discover_timesteps_silo(case_dir) (and add an import for silo_reader
if missing) and return its result, removing the duplicated code so only
discover_timesteps and discover_timesteps_silo remain as single sources of
truth.
- Around line 296-302: The loop that builds proc_data silently skips missing
files and zero-dimension reads; update the block around fpath/read_binary_file
so it emits a warning whenever a processor is skipped: when
os.path.isfile(fpath) is false log a warning including rank, fpath, step and
var; after calling read_binary_file, if pdata.m == 0 and pdata.n == 0 and
pdata.p == 0 log a warning including rank, fpath and the empty dimensions before
continuing; use the module's logger or the logging.warning API to provide clear,
contextual messages referencing fpath, rank, step, var and pdata so users can
diagnose missing/empty processor files.
- Around line 47-62: The free-standing read_record function is unused and should
either be removed or renamed to _read_record with a comment explaining it's a
convenience wrapper only used for ad-hoc reads; if you keep it, change its
behavior to detect and return endianness (or delegate to the existing
_read_record_endian) and validate the trailing 4-byte marker matches the leading
marker before returning the payload to avoid silent corruption; update
references to use _read_record or remove the function entirely and ensure no
other code relies on read_record.
In `@toolchain/mfc/viz/silo_reader.py`:
- Around line 126-132: Add a brief inline comment next to the Fortran-order
reinterpretation in silo_reader.py (the block that does data =
np.ascontiguousarray(data).ravel().reshape(data.shape, order='F') and assigns
into variables[key]) stating the assumption that the HDF5/Silo dataset shape
preserves the Fortran dimension ordering (nx, ny, nz), and note that if that
ordering ever changes the reshape will silently transpose data; optionally add a
runtime sanity check (shape or known axis order) or a warning log to detect
unexpected dimension-order changes before assigning to variables[key].
- Around line 42-49: The helper _read_silo_object is dead code or duplicated
logic; either remove it or update read_silo_file to call it instead of repeating
type and attribute checks. Locate the two places in read_silo_file where the
code checks isinstance(obj, h5py.Datatype) and accesses obj.attrs["silo"]
(currently duplicated) and replace that block with a call to
_read_silo_object(obj) and handle a None return (skip/raise) the same way the
inline code currently does; alternatively, delete the unused _read_silo_object
function if you prefer to keep the inline checks.
In `@toolchain/mfc/viz/viz.py`:
- Around line 14-34: The single-int branch in _parse_steps doesn't validate
step_arg against available_steps; update the final return path to convert
step_arg to int, check membership against available_steps (or its set) and
return [that_int] only if present, otherwise return an empty list (matching the
range filtering behavior) so callers get consistent, pre-validated timesteps.
In `@toolchain/pyproject.toml`:
- Around line 40-43: Update pyproject.toml to pin lower bounds for the image I/O
packages and move the heavy ffmpeg wheel into an optional extra: add a minimum
version constraint for "imageio" in the dependencies list (e.g.,
"imageio>=2.33") and remove "imageio-ffmpeg" from the main dependencies, then
add an [project.optional-dependencies] (or equivalent) section named something
like "viz-video" containing "imageio-ffmpeg>=0.5.0" so users can opt into the
~60 MiB binary only when they need MP4 generation.
![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 20 files
Confidence score: 3/5
- There is a concrete user-impacting risk: in
toolchain/mfc/viz/renderer.py, log-scale rendering can crash whenhiis non‑positive, which would abort CLI rendering for some fields. - The reported issue appears twice but points to the same underlying log-scale guard gap, so the impact is localized yet real.
- Pay close attention to
toolchain/mfc/viz/renderer.py- log-scale handling for non‑positive data can throw and halt rendering.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="toolchain/mfc/viz/renderer.py">
<violation number="1" location="toolchain/mfc/viz/renderer.py:48">
P2: Guard log-scale against non‑positive data. As written, `hi` can be <= 0, causing LogNorm to throw and the CLI to crash for fields with non‑positive values.</violation>
<violation number="2" location="toolchain/mfc/viz/renderer.py:110">
P2: Guard log-scale against non‑positive slice data; `hi` can be <= 0, which makes LogNorm error and aborts rendering.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Pull request overview
This pull request adds a built-in visualization command (./mfc.sh viz) to MFC's toolchain, enabling users to render PNG images and MP4 videos directly from post-processed simulation output without requiring external GUI tools like ParaView or VisIt.
Changes:
- Adds
mfc/viz/package with binary and Silo-HDF5 readers, matplotlib-based renderer, and CLI entry point - Integrates new
vizcommand into MFC's CLI system - Adds
imageioandimageio-ffmpegdependencies for portable MP4 generation - Renames existing
mfc.viztomfc.viz_legacyfor backward compatibility - Updates documentation with comprehensive usage guide and troubleshooting
Reviewed changes
Copilot reviewed 18 out of 20 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
toolchain/pyproject.toml |
Adds imageio dependencies for video rendering |
toolchain/mfc/viz_legacy.py |
Legacy visualization API renamed from mfc.viz for backward compatibility |
toolchain/mfc/viz/viz.py |
Main CLI entry point; handles argument parsing and dispatching |
toolchain/mfc/viz/reader.py |
Binary format reader with endianness/precision auto-detection |
toolchain/mfc/viz/silo_reader.py |
Silo-HDF5 format reader using h5py |
toolchain/mfc/viz/renderer.py |
Matplotlib-based rendering for 1D/2D/3D visualizations and MP4 videos |
toolchain/mfc/viz/__init__.py |
Empty package init (new package) |
toolchain/mfc/cli/commands.py |
VIZ_COMMAND definition with arguments and examples |
toolchain/mfc/args.py |
Adds viz to relevant_subparsers list |
toolchain/main.py |
Integrates viz command into dispatch and skips CMake check |
examples/*/viz.py, examples/*/export.py, examples/*/analyze.py |
Updates imports from mfc.viz to mfc.viz_legacy |
docs/documentation/visualization.md |
Comprehensive viz command documentation |
docs/documentation/troubleshooting.md |
Viz-specific troubleshooting section |
docs/documentation/running.md |
Cross-reference to viz command |
docs/documentation/getting-started.md |
Quick start viz examples |
docs/documentation/case.md |
Notes on format compatibility with viz |
.typos.toml |
Adds "PNGs" to allowed words list |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
toolchain/mfc/viz/reader.py (2)
370-379: Addstacklevel=2towarnings.warncalls.Per Python best practices (and flagged by Ruff B028),
warnings.warnwithout an explicitstacklevelpoints the warning at thewarn()call itself rather than the caller.stacklevel=2makes the warning message more useful for debugging.♻️ Proposed fix
- warnings.warn(f"Processor file not found, skipping: {fpath}") + warnings.warn(f"Processor file not found, skipping: {fpath}", stacklevel=2) continue pdata = read_binary_file(fpath, var_filter=var) if pdata.m == 0 and pdata.n == 0 and pdata.p == 0: import warnings # pylint: disable=import-outside-toplevel - warnings.warn(f"Processor p{rank} has zero dimensions, skipping") + warnings.warn(f"Processor p{rank} has zero dimensions, skipping", stacklevel=2)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/viz/reader.py` around lines 370 - 379, The two warnings.warn calls in the loop that builds fpath and checks pdata dimensions should include stacklevel=2 so the warning is attributed to the caller instead of the warn call; locate the calls that warn "Processor file not found, skipping: {fpath}" and "Processor p{rank} has zero dimensions, skipping" (within the loop that constructs fpath and calls read_binary_file) and add stacklevel=2 to each warnings.warn invocation.
70-80: Trailing record marker is consumed but not verified.Line 79 reads the trailing 4-byte marker but doesn't check that it matches the leading marker. For robustness against corrupted files, consider verifying:
trailing = f.read(4) if len(trailing) == 4: trail_len = struct.unpack(f'{endian}i', trailing)[0] if trail_len != rec_len: raise ValueError(f"Record marker mismatch: {rec_len} vs {trail_len}")This is optional — most Fortran readers skip this check, and corrupted files are rare.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/viz/reader.py` around lines 70 - 80, In _read_record_endian, after reading the trailing 4 bytes (currently f.read(4)), read and verify the trailing marker instead of discarding it: ensure you check the trailing marker length is 4, unpack it with the same endian as used for the leading marker, compare the unpacked trail_len to rec_len, and raise a ValueError (e.g., "Record marker mismatch: {rec_len} vs {trail_len}") if they differ; also raise an EOFError if the trailing read returns fewer than 4 bytes to mirror the earlier checks.toolchain/mfc/viz/viz.py (1)
156-174: First timestep is read twice — once for validation, once for rendering.
read_step_all_vars(requested_steps[0])at line 169 reads all variables for validation, then the rendering loop reads the same step again (with var filter) at line 204. For large datasets this doubles I/O for the first frame. Consider caching or reusing the already-loaded data for the first frame.This is a minor optimization and not blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/viz/viz.py` around lines 156 - 174, The first timestep is read twice via read_step_all_vars(requested_steps[0]) for validation and later via read_step in the rendering loop; change the code to cache test_assembled and reuse it for the first frame instead of calling read_step again: after computing test_assembled (using assemble_silo or assemble) store it in a variable (e.g., cached_first = test_assembled) and in the rendering loop check if current step == requested_steps[0] then use cached_first.variables / cached_first data rather than invoking read_step (which calls assemble_silo/assemble), otherwise call read_step as before; keep existing symbols read_step, read_step_all_vars, assemble_silo, assemble, test_assembled, requested_steps, and varname to locate code.toolchain/mfc/viz/renderer.py (1)
46-52: Consider extracting the duplicated LogNorm computation into a small helper.The same pattern — pick
lofromvminor positive-data minimum (fallback1e-10), pickhifromvmaxor data max, then buildLogNorm— appears identically inrender_2dandrender_3d_slice. A tiny helper (e.g.,_build_log_norm(data, vmin, vmax)) would remove the duplication and make it easier to evolve (e.g., handling all-NaN data more gracefully).Also applies to: 108-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/viz/renderer.py` around lines 46 - 52, Extract the duplicated LogNorm construction into a helper (e.g., _build_log_norm) and call it from render_2d and render_3d_slice: implement _build_log_norm(data, vmin, vmax) to compute lo = vmin if provided else np.nanmin(data[data > 0]) if any positive values else 1e-10, hi = vmax if provided else np.nanmax(data), return LogNorm(vmin=lo, vmax=hi); then replace the inline blocks that set norm = LogNorm(...) and vmin = vmax = None with norm = _build_log_norm(data, vmin, vmax) and clear vmin/vmax as before. Ensure the helper handles NaNs consistently as in the original logic.toolchain/mfc/viz/silo_reader.py (1)
158-163: Missing processor files are silently skipped — inconsistent with binary reader.The binary reader (
reader.pyline 373) emitswarnings.warn(...)when a processor file is not found. Here the silo reader silentlycontinues past missing.silofiles. For consistency and debuggability, consider adding a similar warning.♻️ Suggested fix
silo_file = os.path.join(base, f"p{rank}", f"{step}.silo") if not os.path.isfile(silo_file): + import warnings # pylint: disable=import-outside-toplevel + warnings.warn(f"Processor file not found, skipping: {silo_file}", stacklevel=2) continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/viz/silo_reader.py` around lines 158 - 163, The loop that builds proc_data in silo_reader.py currently skips missing per-rank files silently; update it to warn like the binary reader does by emitting warnings.warn when a constructed silo_file (os.path.join(base, f"p{rank}", f"{step}.silo")) is not found. Ensure warnings is imported if absent, and include contextual info (rank, silo_file, step) in the message so callers of read_silo_file/read_silo_files can diagnose missing processor files; keep the continue behavior after warning and leave read_silo_file and proc_data usage unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/documentation/visualization.md`:
- Around line 43-47: Update the documentation table entry for the Range `--step`
format to indicate that the end value is inclusive (the code uses range(start,
end + 1, stride)), e.g., change the Description for the Range row (`--step
0:10000:500`) to include "(inclusive)" so readers know the end value is
included.
In `@toolchain/mfc/viz/viz.py`:
- Around line 14-37: The _parse_steps function should validate and handle
malformed step_arg instead of letting int() raise raw ValueError: wrap parsing
of start/end/stride and the single-int conversion in a try/except that raises a
clean ValueError with a descriptive message referencing the provided step_arg
and expected formats (function: _parse_steps, params: step_arg,
available_steps); then update the caller viz to wrap its call to _parse_steps in
a try/except that catches ValueError (when computing requested_steps) and prints
a user-friendly error via cons.print and exits with sys.exit(1).
- Around line 90-94: The code crashes when step_arg == "all": change the branch
handling step_arg so that if step_arg is None or step_arg.lower() == "all" you
set step = steps[0] and call cons.print(f"[dim]Using first available timestep:
{step}[/dim]"); otherwise attempt to parse step = int(step_arg) inside a
try/except catching ValueError and emit a clear error via cons.print (or raise a
user-facing exception) mentioning the invalid step_arg. Update the logic around
the variables step_arg, steps, step and the existing cons.print call to
implement this behavior.
---
Nitpick comments:
In `@toolchain/mfc/viz/reader.py`:
- Around line 370-379: The two warnings.warn calls in the loop that builds fpath
and checks pdata dimensions should include stacklevel=2 so the warning is
attributed to the caller instead of the warn call; locate the calls that warn
"Processor file not found, skipping: {fpath}" and "Processor p{rank} has zero
dimensions, skipping" (within the loop that constructs fpath and calls
read_binary_file) and add stacklevel=2 to each warnings.warn invocation.
- Around line 70-80: In _read_record_endian, after reading the trailing 4 bytes
(currently f.read(4)), read and verify the trailing marker instead of discarding
it: ensure you check the trailing marker length is 4, unpack it with the same
endian as used for the leading marker, compare the unpacked trail_len to
rec_len, and raise a ValueError (e.g., "Record marker mismatch: {rec_len} vs
{trail_len}") if they differ; also raise an EOFError if the trailing read
returns fewer than 4 bytes to mirror the earlier checks.
In `@toolchain/mfc/viz/renderer.py`:
- Around line 46-52: Extract the duplicated LogNorm construction into a helper
(e.g., _build_log_norm) and call it from render_2d and render_3d_slice:
implement _build_log_norm(data, vmin, vmax) to compute lo = vmin if provided
else np.nanmin(data[data > 0]) if any positive values else 1e-10, hi = vmax if
provided else np.nanmax(data), return LogNorm(vmin=lo, vmax=hi); then replace
the inline blocks that set norm = LogNorm(...) and vmin = vmax = None with norm
= _build_log_norm(data, vmin, vmax) and clear vmin/vmax as before. Ensure the
helper handles NaNs consistently as in the original logic.
In `@toolchain/mfc/viz/silo_reader.py`:
- Around line 158-163: The loop that builds proc_data in silo_reader.py
currently skips missing per-rank files silently; update it to warn like the
binary reader does by emitting warnings.warn when a constructed silo_file
(os.path.join(base, f"p{rank}", f"{step}.silo")) is not found. Ensure warnings
is imported if absent, and include contextual info (rank, silo_file, step) in
the message so callers of read_silo_file/read_silo_files can diagnose missing
processor files; keep the continue behavior after warning and leave
read_silo_file and proc_data usage unchanged.
In `@toolchain/mfc/viz/viz.py`:
- Around line 156-174: The first timestep is read twice via
read_step_all_vars(requested_steps[0]) for validation and later via read_step in
the rendering loop; change the code to cache test_assembled and reuse it for the
first frame instead of calling read_step again: after computing test_assembled
(using assemble_silo or assemble) store it in a variable (e.g., cached_first =
test_assembled) and in the rendering loop check if current step ==
requested_steps[0] then use cached_first.variables / cached_first data rather
than invoking read_step (which calls assemble_silo/assemble), otherwise call
read_step as before; keep existing symbols read_step, read_step_all_vars,
assemble_silo, assemble, test_assembled, requested_steps, and varname to locate
code.
codeant-ai
bot
added
the
size:XXL
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)
docs/documentation/visualization.md (1)
265-265:⚠️ Potential issue | 🟡 MinorStale "last updated" date.
The page footer says 2026-02-13 but this PR is dated 2026-02-22. Consider updating to match.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/documentation/visualization.md` at line 265, Update the stale page footer date in the HTML footer div (<div style='text-align:center; font-size:0.75rem; color:`#888`; padding:16px 0 0;'>Page last updated: 2026-02-13</div>) to the current PR date (2026-02-22) so the "Page last updated" text matches the PR date.
🧹 Nitpick comments (3)
toolchain/mfc/viz/renderer.py (2)
46-56: Duplicated log-scale normalization logic inrender_2dandrender_3d_slice.The log-scale guard block (compute
lo/hi, clamp non-positive values, constructLogNorm, clearvmin/vmax) is identical in both functions. Consider extracting a small helper like_build_log_norm(data, vmin, vmax)that returns(norm, vmin, vmax).Not blocking — the duplication is contained to two call sites.
♻️ Example helper extraction
def _build_log_norm(data, vmin, vmax): """Build a LogNorm from data, clamping non-positive bounds.""" lo = vmin if vmin is not None else (np.nanmin(data[data > 0]) if np.any(data > 0) else 1e-10) hi = vmax if vmax is not None else np.nanmax(data) if hi <= 0: hi = 1.0 if lo <= 0 or lo >= hi: lo = hi * 1e-10 return LogNorm(vmin=lo, vmax=hi), None, NoneThen in both
render_2dandrender_3d_slice:if log_scale: norm, vmin, vmax = _build_log_norm(data, vmin, vmax)Also applies to: 112-122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/viz/renderer.py` around lines 46 - 56, Extract the duplicated log-scale normalization block into a small helper function (e.g., _build_log_norm(data, vmin, vmax)) and replace the identical logic in both render_2d and render_3d_slice with a call that assigns its return values; specifically, move the lo/hi computation, non-positive clamps, LogNorm construction, and clearing of vmin/vmax into _build_log_norm so each call becomes: if log_scale: norm, vmin, vmax = _build_log_norm(data, vmin, vmax) while keeping the helper signature and return values consistent with the current usage in renderer.py.
159-181: Auto vmin/vmax uses global 3D range, not slice range — document or note for 3D.For 3D data, the sampling at lines 173-176 computes
nanmin/nanmaxover the entire 3D volume, but the rendered frames are 2D slices. The auto-computed color range may be wider than what appears in the slice, potentially producing washed-out colors. This is a reasonable default (consistent color scale across a video), but worth noting. The--vmin/--vmaxoverride covers users who need tighter scaling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/viz/renderer.py` around lines 159 - 181, The auto vmin/vmax computation currently takes nanmin/nanmax over full 3D volumes (see sample_steps loop using read_func and d = ad.variables.get(varname) then np.nanmin/np.nanmax) which can produce a wider color range than individual 2D slices; add a brief code comment and a user-facing note/warning when d.ndim == 3 (or when sample data is 3D) explaining that the computed vmin/vmax are global 3D ranges and may appear washed-out on per-slice renders, and mention that users can override with --vmin/--vmax (or keep an option to switch to per-slice scaling if desired).toolchain/mfc/viz/viz.py (1)
120-137: No--stepdefaults to rendering every available timestep — consider a warning or explicit opt-in.When
step_argisNone(user omits--step),_parse_steps(None, steps)returns all available steps (line 23-24). For simulations with thousands of timesteps this silently generates thousands of PNGs with no confirmation. All documentation examples show--stepexplicitly.Consider either requiring
--stepfor rendering or printing a warning like"No --step specified — rendering all N timesteps. Continue? (use --step all to suppress)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/viz/viz.py` around lines 120 - 137, The current flow silently renders all timesteps when step_arg is None; update the logic around ARG('step') / step_arg and _parse_steps to avoid accidental huge renders by detecting when step_arg is None and _parse_steps would return all steps: after discover_timesteps(...) compute steps_count, and if step_arg is None then either (preferred) prompt the user with cons.print asking "No --step specified — rendering all N timesteps. Continue? (use --step all to suppress)" and abort (sys.exit(1)) unless the user confirms, or require explicit --step and exit with that warning; reference ARG, step_arg, _parse_steps, requested_steps, discover_timesteps and use cons.print for the message and sys.exit on negative response.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@toolchain/mfc/viz/viz.py`:
- Around line 61-68: Wrap the call to discover_format(case_dir) (used when
fmt_arg is falsy) in a try/except that catches FileNotFoundError, and on that
exception print a clean CLI error via cons.print (or similar) including the
exception message and then exit non‑zero (e.g., sys.exit(1)); keep the existing
behavior of using fmt_arg when present, and only apply the try/except around the
discover_format call referenced where fmt_arg and fmt are set.
---
Outside diff comments:
In `@docs/documentation/visualization.md`:
- Line 265: Update the stale page footer date in the HTML footer div (<div
style='text-align:center; font-size:0.75rem; color:`#888`; padding:16px 0 0;'>Page
last updated: 2026-02-13</div>) to the current PR date (2026-02-22) so the "Page
last updated" text matches the PR date.
---
Nitpick comments:
In `@toolchain/mfc/viz/renderer.py`:
- Around line 46-56: Extract the duplicated log-scale normalization block into a
small helper function (e.g., _build_log_norm(data, vmin, vmax)) and replace the
identical logic in both render_2d and render_3d_slice with a call that assigns
its return values; specifically, move the lo/hi computation, non-positive
clamps, LogNorm construction, and clearing of vmin/vmax into _build_log_norm so
each call becomes: if log_scale: norm, vmin, vmax = _build_log_norm(data, vmin,
vmax) while keeping the helper signature and return values consistent with the
current usage in renderer.py.
- Around line 159-181: The auto vmin/vmax computation currently takes
nanmin/nanmax over full 3D volumes (see sample_steps loop using read_func and d
= ad.variables.get(varname) then np.nanmin/np.nanmax) which can produce a wider
color range than individual 2D slices; add a brief code comment and a
user-facing note/warning when d.ndim == 3 (or when sample data is 3D) explaining
that the computed vmin/vmax are global 3D ranges and may appear washed-out on
per-slice renders, and mention that users can override with --vmin/--vmax (or
keep an option to switch to per-slice scaling if desired).
In `@toolchain/mfc/viz/viz.py`:
- Around line 120-137: The current flow silently renders all timesteps when
step_arg is None; update the logic around ARG('step') / step_arg and
_parse_steps to avoid accidental huge renders by detecting when step_arg is None
and _parse_steps would return all steps: after discover_timesteps(...) compute
steps_count, and if step_arg is None then either (preferred) prompt the user
with cons.print asking "No --step specified — rendering all N timesteps.
Continue? (use --step all to suppress)" and abort (sys.exit(1)) unless the user
confirms, or require explicit --step and exit with that warning; reference ARG,
step_arg, _parse_steps, requested_steps, discover_timesteps and use cons.print
for the message and sys.exit on negative response.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
toolchain/mfc/viz/silo_reader.py (2)
147-169: Consider warning on missing per-rank Silo files to avoid “partial assembly” surprises.In
assemble_silo, missing rank files are silently skipped (Lines 161-164). If a run produced incomplete output, users may unknowingly render an incomplete domain. Even a low-noise warning (once per missing file, withstacklevel=2) would help.Possible tweak
for rank in ranks: silo_file = os.path.join(base, f"p{rank}", f"{step}.silo") if not os.path.isfile(silo_file): + import warnings # pylint: disable=import-outside-toplevel + warnings.warn(f"Missing Silo rank file, skipping: {silo_file}", stacklevel=2) continue pdata = read_silo_file(silo_file, var_filter=var) proc_data.append((rank, pdata))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/viz/silo_reader.py` around lines 147 - 169, The loop that iterates ranks and skips missing per-rank files (the block that builds proc_data by checking os.path.isfile(silo_file) and continue) should emit a low-noise warning for each missing file instead of silently skipping; use warnings.warn with a clear message including the missing path and step (and include stacklevel=2) so users know about partial assemblies, and keep appending existing files via read_silo_file as before (refer to variables ranks, silo_file, step, read_silo_file, and proc_data).
121-130: Data reordering is subtle; add/ensure a regression test for axis/order parity vs binary.The Line 128-130 reshape (
ravel()thenreshape(..., order='F')) encodes a strong assumption about how MFC writes DBPUTQV1 buffers into HDF5 and how h5py materializes them. If this is wrong, plots will look “plausible” but be transposed/permuted—hard to notice without a known-field test.I’d strongly recommend a small golden test that:
- writes a tiny 2D/3D field with a known pattern (e.g.,
f(i,j,k)=i+10*j+100*k),- reads via both
read_binary_fileandread_silo_file,- asserts arrays match exactly (or at least match under the intended orientation).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/viz/silo_reader.py` around lines 121 - 130, The data reshape logic in silo_reader.py (the data.ndim check that does data = np.ascontiguousarray(data).ravel().reshape(data.shape, order='F') and assigns into variables[key]) relies on an assumption about Fortran vs C ordering and can silently transpose axes; add a regression test that creates a tiny 2D and 3D golden field (e.g., f(i,j,k)=i+10*j+100*k), writes it with the binary writer used by read_binary_file and also writes/reads it via read_silo_file (or writes an HDF5/Silo file that mimics DBPUTQV1 layout), then assert exact equality (or equality under the intended orientation) between the arrays returned by read_binary_file and read_silo_file to lock in axis/order parity and catch future regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@toolchain/mfc/viz/reader.py`:
- Around line 55-67: The _detect_endianness function should guard against
empty/truncated files: after reading raw = f.read(4) check if len(raw) < 4 and
raise a clear ValueError (or EOFError) that includes the path argument;
alternatively wrap the struct.unpack calls in a try/except struct.error and
re-raise a ValueError with the filename and a concise message indicating the
file is empty or corrupt. Ensure the raised error mentions the path and keeps
the existing behavior of returning '<' or '>' when detection succeeds.
- Around line 70-82: _in _read_record_endian the trailing Fortran record marker
is read but not validated; change the trailing read to ensure 4 bytes were
returned, unpack it with the same endian (e.g., struct.unpack(f'{endian}i',
trailing)[0]) and compare it to rec_len, and raise an appropriate
EOFError/ValueError if the trailing marker is missing or does not match rec_len
so corrupted/truncated files are detected immediately.
- Around line 85-177: In read_binary_file validate the unpacked header fields
(m, n, p, dbvars) immediately after they are read: ensure m, n, p are
non-negative integers and dbvars is non-negative and within a reasonable cap
(e.g., a max expected vars constant), and raise a clear ValueError if they fail;
use these validated values for subsequent calculations of n_vals and data_size
to avoid bogus allocations and dtype detection failures (check the variables m,
n, p, dbvars, n_vals, data_size, and the function read_binary_file).
- Around line 371-381: The warnings emitted inside the ranks loop (when a
processor file is missing or when pdata has zero dimensions) should include a
stacklevel parameter so the warning points to the caller; update the two
warnings.warn calls inside the loop that trigger for missing fpath and for
pdata.m/n/p == 0 to pass stacklevel=2 (keeping the same message) so users see
the warning originating from their callsite; these occur alongside use of
read_binary_file and variables fpath, pdata, and rank.
- Around line 256-338: The assemble_from_proc_data function silently clips
out-of-range searchsorted indices (np.clip around searchsorted in the
proc_centers loop) which can misplace data — replace clipping with assertions
that xi/yi/zi are within [0, nx-1], [0, ny-1], [0, nz-1] respectively to fail
fast on coordinate mismatches; also build varnames as the union of all
pd.variables across proc_data (instead of using proc_data[0][1].variables) and
initialize global_vars for that union so no per-rank variables are dropped,
keeping the rest of the placement logic (np.ix_ + assignment) unchanged.
In `@toolchain/mfc/viz/silo_reader.py`:
- Around line 43-47: The _resolve_path function must handle numpy.bytes_ values
returned by h5py so the path isn't mangled; update the type check in
_resolve_path to detect and decode numpy.bytes_ (e.g., treat instances of bytes
and numpy.bytes_ the same) or fall back to calling .decode() when available
before constructing the lookup key, then use the decoded string when indexing
h5file (function: _resolve_path).
---
Nitpick comments:
In `@toolchain/mfc/viz/silo_reader.py`:
- Around line 147-169: The loop that iterates ranks and skips missing per-rank
files (the block that builds proc_data by checking os.path.isfile(silo_file) and
continue) should emit a low-noise warning for each missing file instead of
silently skipping; use warnings.warn with a clear message including the missing
path and step (and include stacklevel=2) so users know about partial assemblies,
and keep appending existing files via read_silo_file as before (refer to
variables ranks, silo_file, step, read_silo_file, and proc_data).
- Around line 121-130: The data reshape logic in silo_reader.py (the data.ndim
check that does data = np.ascontiguousarray(data).ravel().reshape(data.shape,
order='F') and assigns into variables[key]) relies on an assumption about
Fortran vs C ordering and can silently transpose axes; add a regression test
that creates a tiny 2D and 3D golden field (e.g., f(i,j,k)=i+10*j+100*k), writes
it with the binary writer used by read_binary_file and also writes/reads it via
read_silo_file (or writes an HDF5/Silo file that mimics DBPUTQV1 layout), then
assert exact equality (or equality under the intended orientation) between the
arrays returned by read_binary_file and read_silo_file to lock in axis/order
parity and catch future regressions.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
toolchain/mfc/viz/reader.py (1)
378-392: Moveimport warningsto module-level imports.
warningsis imported twice inside the loop body (lines 380, 385), suppressed with# pylint: disable=import-outside-toplevel. Moving it to the top-level imports (alongsideos,struct, etc.) removes the suppressions and follows PEP 8's import ordering convention.♻️ Proposed refactor
import os import struct +import warnings from dataclasses import dataclass, fieldThen in
assemble:if not os.path.isfile(fpath): - import warnings # pylint: disable=import-outside-toplevel warnings.warn(f"Processor file not found, skipping: {fpath}", stacklevel=2) continue pdata = read_binary_file(fpath, var_filter=var) if pdata.m == 0 and pdata.n == 0 and pdata.p == 0: - import warnings # pylint: disable=import-outside-toplevel warnings.warn(f"Processor p{rank} has zero dimensions, skipping", stacklevel=2)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/viz/reader.py` around lines 378 - 392, The code in reader.py imports warnings inside the loop where files are checked (around the block that builds proc_data using read_binary_file and appends (rank, pdata)) which causes duplicate local imports and uses pylint disables; move the import to the module-level imports (alongside os, struct, etc.), remove the in-function imports and the "# pylint: disable=import-outside-toplevel" comments, and leave the warnings.warn calls unchanged (e.g., the two warnings.warn(...) calls in the loop should continue to use the module-level warnings name); ensure any linter comments referencing the local import are deleted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@toolchain/mfc/viz/reader.py`:
- Line 83: The trailing Fortran record marker read (currently just `f.read(4)`)
must be validated: after reading the 4 bytes verify that 4 bytes were returned
and that the integer value equals `rec_len`; on failure raise a descriptive
exception (or return an error) to avoid silent desync. Update the code around
the `f.read(4)` call in reader.py to check the length of the returned bytes,
convert them to an integer (matching the same endianness/format used elsewhere),
compare to `rec_len`, and handle mismatches or short reads by raising an
exception that includes expected vs actual lengths.
- Around line 327-329: The np.clip calls masking out-of-range searchsorted
results should be removed and replaced with explicit bounds checks so coordinate
mismatches are surfaced: after computing xi = np.searchsorted(global_x,
np.round(x_cc, 12)) (and similarly yi/zi), assert or raise if any xi < 0 or xi
>= nx (and yi < 0 or yi >= ny when ndim>=2, zi when ndim>=3) so invalid
insertion indices are not silently mapped to 0 or nx-1; use the existing names
xi, yi, zi, global_x/global_y/global_z, np.round and nx/ny/nz to locate the
lines to change and ensure well-formed data fails fast instead of being
corrupted by clipping.
---
Nitpick comments:
In `@toolchain/mfc/viz/reader.py`:
- Around line 378-392: The code in reader.py imports warnings inside the loop
where files are checked (around the block that builds proc_data using
read_binary_file and appends (rank, pdata)) which causes duplicate local imports
and uses pylint disables; move the import to the module-level imports (alongside
os, struct, etc.), remove the in-function imports and the "# pylint:
disable=import-outside-toplevel" comments, and leave the warnings.warn calls
unchanged (e.g., the two warnings.warn(...) calls in the loop should continue to
use the module-level warnings name); ensure any linter comments referencing the
local import are deleted.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="toolchain/mfc/viz/silo_reader.py">
<violation number="1" location="toolchain/mfc/viz/silo_reader.py:164">
P2: Skipping missing processor files can silently return incomplete assembled data. For multi-rank cases, a missing rank should be treated as an error to avoid incorrect visualizations.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1233 +/- ##
=======================================
Coverage 44.04% 44.04%
=======================================
Files 70 70
Lines 20499 20499
Branches 1993 1993
=======================================
Hits 9028 9028
Misses 10330 10330
Partials 1141 1141 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
codeant-ai
bot
added
size:XXL
sbryngelson
force-pushed
the
viz-fast
branch
from
0214571 to
29f20a9
Compare
codeant-ai
bot
removed
the
size:XXL
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Claude Code ReviewHead SHA: 5b8d16e Files changed: 54 Key files (10 of 54)
Summary
Findings1. Dependencies are not optional despite PR description saying so —The PR description says:
But the actual diff adds them to the main + # Visualization (./mfc.sh viz)
+ "matplotlib",
+ "h5py",
+ "imageio>=2.33",
+ "imageio-ffmpeg>=0.5.0",
+ "plotext>=5.2.0",
+ "textual>=0.47.0",
+ "textual-plotext>=0.2.0",
+ "dash>=2.0",
+ "plotly",
If the intent is optional extras, the correct fix is: [project.optional-dependencies]
viz = ["matplotlib", "h5py", "imageio>=2.33", "imageio-ffmpeg>=0.5.0",
"plotext>=5.2.0", "textual>=0.47.0", "textual-plotext>=0.2.0",
"dash>=2.0", "plotly", "tqdm"]…with auto-install logic in 2.
|
Claude Code ReviewHead SHA: 9491519 Key new files:
Summary
Findings1. MEDIUM —
|
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Claude Code ReviewHead SHA: a49ceba Files changed: 54 (4,843 additions / 494 deletions) Key files:
Summary
Findings1.
|
Claude Code ReviewHead SHA: Key files (top 10)
Summary
Findings1. Medium —
|
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
TUI and interactive modes have their own UI controls for colormap, vmin/vmax, log scale, and slice position, so those CLI flags are only needed for batch rendering. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Claude Code ReviewHead SHA: Files changed: 55 (key paths)
Summary
Findings1. The PR description says they go into 2.
3.
4. # restart support is not yet implemented.
skip = 1 + step * nBubsThe limitation is documented, but for a restarted simulation the code silently reads the wrong bubble block rather than failing. Recommend emitting a 5. data = np.ascontiguousarray(data).ravel().reshape(data.shape, order='F')The comment correctly flags that if HDF5 ever reverses the shape ordering, multi-D silo data will be silently transposed. The fixture files are MFC-generated, so they can't catch this assumption failing. A test that verifies a known spatial gradient in a fixture variable would provide stronger assurance. 6. (r'^vel(\d+)$', lambda m: [r'$u$', r'$v$', r'$w$'][int(m.group(1)) - 1]
if int(m.group(1)) <= 3 else rf'$v_{m.group(1)}$'),
7. Standard for Dash apps with dynamic IDs, but unexpected exceptions in future callbacks will vanish silently. The current Improvement opportunities
|
Claude Code ReviewHead SHA: 64257b4 Files changed: 55 (4,848 additions / 496 deletions) Key files:
Summary
Findings1. PR description contradicts actual dependency placement — importantThe PR says:
But # Visualization (./mfc.sh viz)
"matplotlib",
"h5py",
"imageio>=2.33",
"imageio-ffmpeg>=0.5.0",
"plotext>=5.2.0",
"textual>=0.47.0",
"textual-plotext>=0.2.0",
"dash>=2.0",
"plotly",
"tqdm",These packages (especially 2.
|
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Claude Code ReviewHead SHA: ced26e4 Key files:
Summary
FindingsP1 — Correctness / breakage1.
2. PR description says deps are optional — they are not ( The PR description states: "Viz-specific dependencies moved to P2 — Behavioural3. Silent # viz.py (TUI + interactive branch)
if ARG('interactive') or use_tui or ARG('mp4'):
if step_arg == 'last':
step_arg = 'all' # silently loads EVERY timestepThe CLI default for 4. Lagrange bubble reader gives wrong results for restarted simulations, silently ( # reader.py ~line 548
# MFC writes one bubble block per simulation timestep...
# For restarts where t_step_start>0 the lag file starts at 0
# but step numbers begin at t_step_start — seeking would overshoot;
# restart support is not yet implemented.
skip = 1 + step * nBubsThe comment acknowledges that restart simulations will silently produce incorrect bubble overlays (off-by- 5. Silo reshape assumption could silently transpose 2-D/3-D data ( if data.ndim >= 2:
data = np.ascontiguousarray(data).ravel().reshape(data.shape, order='F')
# Assumption: Silo/HDF5 preserves the Fortran dimension ordering
# (nx, ny, nz) as the dataset shape. If a future Silo version
# reverses the shape, this reshape would silently transpose data.This assumption is currently unverified in the test suite (the 2D/3D silo tests check P3 — Minor6.
7.
8. A first pass over all frame files finds the max dimensions; a second pass encodes. For a 500-frame render this doubles PNG I/O. Fixing the output Overall assessmentThe core reading/assembly logic and the TUI/interactive UI are well-implemented and well-tested for the happy path. The two blocking items are the |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
Claude Code ReviewHead SHA:
Summary
Findings1. Viz unit tests added to python3 -m unittest mfc.viz.test_viz -vThe viz tests require optional deps ( 2. 3. Lagrange bubble step alignment is not verified after seeking skip = 1 + step * nBubsAfter seeking, no check is made that the first column (simulation time or step index) of the rows read actually corresponds to step. If 4. 5. Silent try:
matplotlib.use('Agg')
except ValueError:
passIf matplotlib is already initialized (e.g., in a test that imported matplotlib before renderer), the backend remains whatever was set earlier and renders may open GUI windows or fail silently. The 6. (r'^vel(\d+)$', lambda m: [r'$u$', r'$v$', r'$w$'][int(m.group(1)) - 1] ...)If Improvement Opportunities (no blocking issues)
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| def _advance_step(_, current, loop_val): | ||
| try: | ||
| idx = steps.index(current) | ||
| except ValueError: | ||
| idx = 0 | ||
| nxt = idx + 1 | ||
| if nxt >= len(steps): | ||
| return steps[0] if ('loop' in (loop_val or [])) else no_update | ||
| return steps[nxt] |
There was a problem hiding this comment.
Suggestion: Fix a bug in _advance_step that causes an infinite loop during playback. Modify the function to stop the playback timer when it reaches the end of the steps and looping is disabled. [possible issue, importance: 8]
| def _advance_step(_, current, loop_val): | |
| try: | |
| idx = steps.index(current) | |
| except ValueError: | |
| idx = 0 | |
| nxt = idx + 1 | |
| if nxt >= len(steps): | |
| return steps[0] if ('loop' in (loop_val or [])) else no_update | |
| return steps[nxt] | |
| @app.callback( | |
| Output('step-sel', 'value'), | |
| Output('playing-st', 'data', allow_duplicate=True), | |
| Input('play-iv', 'n_intervals'), | |
| State('step-sel', 'value'), | |
| State('loop-chk', 'value'), | |
| prevent_initial_call=True, | |
| ) | |
| def _advance_step(_, current, loop_val): | |
| try: | |
| idx = steps.index(current) | |
| except ValueError: | |
| idx = 0 | |
| nxt = idx + 1 | |
| if nxt >= len(steps): | |
| if 'loop' in (loop_val or []): | |
| return steps[0], no_update | |
| return no_update, False # Stop playback | |
| return steps[nxt], no_update |
| ) | ||
|
|
||
| # Load first step to know dimensionality and available variables | ||
| init = _load(steps[0], read_func) |
There was a problem hiding this comment.
Suggestion: Prevent a potential IndexError in run_interactive by checking if the steps list is empty before accessing its first element. [possible issue, importance: 6]
| init = _load(steps[0], read_func) | |
| if not steps: | |
| from mfc.common import MFCException | |
| raise MFCException("No steps provided for visualization") | |
| init = _load(steps[0], read_func) |
| # Preload first step to discover variables | ||
| first = _load(steps[0], read_func) |
There was a problem hiding this comment.
Suggestion: Prevent a potential IndexError in run_tui by checking if the steps list is empty before accessing its first element. [possible issue, importance: 6]
| # Preload first step to discover variables | |
| first = _load(steps[0], read_func) | |
| # Preload first step to discover variables | |
| if not steps: | |
| raise MFCException("No steps provided for TUI visualization") | |
| first = _load(steps[0], read_func) |
| def _cleanup(): | ||
| for fname in sorted(f for f in os.listdir(viz_dir) if f.endswith('.png')): | ||
| try: | ||
| os.remove(os.path.join(viz_dir, fname)) | ||
| except OSError: | ||
| pass | ||
| try: | ||
| os.rmdir(viz_dir) | ||
| except OSError: | ||
| pass |
There was a problem hiding this comment.
Suggestion: Replace the manual file and directory removal in the _cleanup function with a single, more robust call to shutil.rmtree(viz_dir, ignore_errors=True). [general, importance: 6]
| def _cleanup(): | |
| for fname in sorted(f for f in os.listdir(viz_dir) if f.endswith('.png')): | |
| try: | |
| os.remove(os.path.join(viz_dir, fname)) | |
| except OSError: | |
| pass | |
| try: | |
| os.rmdir(viz_dir) | |
| except OSError: | |
| pass | |
| def _cleanup(): | |
| import shutil # pylint: disable=import-outside-toplevel | |
| shutil.rmtree(viz_dir, ignore_errors=True) |
| def _dedup(arr): | ||
| extent = arr.max() - arr.min() | ||
| if extent > 0: | ||
| origin = arr.min() | ||
| norm = np.round((arr - origin) / extent, 12) | ||
| return origin + np.unique(norm) * extent, origin, extent | ||
| return np.unique(arr), arr.min(), 0.0 |
There was a problem hiding this comment.
Suggestion: In the _dedup function, filter out non-finite values from the input array arr before calculating extent to prevent potential division-by-zero errors. [possible issue, importance: 8]
| def _dedup(arr): | |
| extent = arr.max() - arr.min() | |
| if extent > 0: | |
| origin = arr.min() | |
| norm = np.round((arr - origin) / extent, 12) | |
| return origin + np.unique(norm) * extent, origin, extent | |
| return np.unique(arr), arr.min(), 0.0 | |
| def _dedup(arr): | |
| finite_arr = arr[np.isfinite(arr)] | |
| if finite_arr.size == 0: | |
| return np.array([]), 0.0, 0.0 | |
| extent = finite_arr.max() - finite_arr.min() | |
| if extent > 0: | |
| origin = finite_arr.min() | |
| norm = np.round((finite_arr - origin) / extent, 12) | |
| return origin + np.unique(norm) * extent, origin, extent | |
| return np.unique(finite_arr), finite_arr.min(), 0.0 |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (12)
📒 Files selected for processing (43)
📝 WalkthroughWalkthroughThis change introduces a comprehensive rewrite of MFC's visualization system. It replaces the previous file-based Python visualization scripts with a unified, CLI-integrated visualization infrastructure accessible via 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 |
Claude Code ReviewHead SHA: e72dfd9 Key files:
Summary:
FindingsBug:
|
Summary
Adds
./mfc.sh viz— a built-in CLI visualization command for rendering post-processed MFC output directly from the terminal. No ParaView or VisIt required.Features
format=2) and Silo-HDF5 (format=1) with automatic detection--interactivelaunches a Dash/Plotly web UI with live controls for slice position, isosurface thresholds, volume rendering, colormap, log scale, vmin/vmax, variable switching, and timestep playback--tuilaunches a fully keyboard-driven terminal UI (works over SSH, no browser/port-forwarding needed) with:spaceautoplay,,/.step navigation,↑/↓variable selectionllog scale toggle,ffreeze color range,ccycle colormap--cmap,--vmin/vmax,--dpi,--log-scale,--slice-axis/value/index,--mp4,--fps--list-stepsand--list-varsto inspect available data before plottingUsage
Architecture
Other changes
VIZ_COMMANDincommands.py, dispatch inmain.py(skips cmake check)toolchain/mfc/viz.py(oldmfc.viz.CaseAPI) and its dependent example scripts (nD_perfect_reactor/analyze.py,nD_perfect_reactor/export.py,1D_reactive_shocktube/viz.py,1D_inert_shocktube/viz.py) removed; superseded by./mfc.sh vizvisualization.md, added quick-start togetting-started.md, viz examples inrunning.md, troubleshooting sectionrequires-python = ">=3.10"added topyproject.toml[project.optional-dependencies] viz— not installed by default, auto-installed on first./mfc.sh vizrunDependencies
imageio+imageio-ffmpegfor portable MP4 encodingh5pyfor Silo-HDF5 readingdash>=2.0+plotlyfor interactive modetextual>=0.47.0+textual-plotext+plotextfor TUI modeCorrectness notes
_step_cacheuses athreading.Lockfor safe concurrent access from Dash callbacks; reads happen before eviction so a failed read never discards a valid cache entry--stepis honoured so users can limit the loaded set for large 3D cases that would otherwise exceed the 50-step memory limitKeyboardInterruptstill triggers temp-dir cleanupTest plan
./mfc.sh viz <1d_case> --var pres --step 0./mfc.sh viz <2d_case> --var pres --step 0:1000:100 --mp4./mfc.sh viz <3d_case> --var pres --step 0 --slice-axis z./mfc.sh viz <silo_case> --var pres --step 0./mfc.sh viz <case> --interactive./mfc.sh viz <case> --tui--list-varsand--list-stepsinfo commands