Add CLI visualization command (./mfc.sh viz)#1233
Open
sbryngelson wants to merge 18 commits intoMFlowCode:masterfrom
Open
Add CLI visualization command (./mfc.sh viz)#1233sbryngelson wants to merge 18 commits intoMFlowCode:masterfrom
sbryngelson wants to merge 18 commits intoMFlowCode:masterfrom
Conversation
…tput
Adds a new `viz` subcommand that reads MFC binary (and optionally Silo-HDF5)
post-processed output and renders 1D line plots, 2D colormaps, 3D slices,
and MP4 videos directly from the command line with no GUI required.
New files:
- toolchain/mfc/viz/{__init__,reader,silo_reader,renderer,viz}.py
- toolchain/mfc/viz_legacy.py (renamed from toolchain/mfc/viz.py)
Modified files:
- toolchain/mfc/cli/commands.py (VIZ_COMMAND definition)
- toolchain/main.py (dispatch + skip cmake check for viz)
- toolchain/mfc/args.py (add viz to relevant_subparsers)
- examples/{1D_inert_shocktube,1D_reactive_shocktube}/viz.py (update imports)
- examples/nD_perfect_reactor/{analyze,export}.py (update imports)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Validate that the requested variable exists before rendering, showing available variables on error instead of a KeyError traceback. Also fix pylint warnings and typos checker false positives. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
render_mp4 was writing temp frames to case_dir/viz/_frames/ even when --output pointed elsewhere. Now writes frames next to the output file. Also removed unused case_dir parameter from render_mp4 and return success/failure status so the caller can skip the "Done" message when ffmpeg is unavailable. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The silo_reader was looking for coordinate/variable data as HDF5 Groups and Datasets, but MFC's Silo files store objects as HDF5 Named Datatypes with a compound "silo" attribute containing metadata (mesh name, data paths, dimensions). Actual data arrays live under the .silo/ group. Rewrite the reader to: - Find mesh by silo_type=130 (DB_QUADMESH) on Named Datatypes - Find variables by silo_type=501 (DB_QUADVAR) on Named Datatypes - Resolve coord0/coord1/value0 paths from silo attribute to .silo/ datasets - Fix timestep discovery to match actual file naming (<step>.silo) - Clean up multi-processor assembly logic Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Three issues fixed: 1. Silo reader: reinterpret HDF5 data from C row-major to Fortran column-major order so data[i,j,k] maps to (x_i, y_j, z_k) 2. Multi-processor assembly: use per-cell searchsorted + np.ix_ indexing instead of contiguous block placement, correctly handling ghost/buffer cell overlap between processors 3. Renderer: fall back to GIF via Pillow when ffmpeg is unavailable Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds imageio and imageio-ffmpeg as dependencies, which bundles a self-contained ffmpeg binary. Replaces subprocess ffmpeg call and Pillow GIF fallback with imageio's get_writer API. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a CLI-first visualization subsystem: new Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as CLI (./mfc.sh viz)
participant Viz as Viz Dispatcher
participant Reader as Reader (binary / silo)
participant Renderer as Renderer
participant FS as Filesystem
User->>CLI: run viz with args
CLI->>Viz: invoke viz()
Viz->>Viz: detect format, discover timesteps & vars
alt list-only
Viz-->>User: print timesteps/vars
else render
Viz->>Reader: read step (read_binary_file / read_silo_file)
Reader-->>Viz: ProcessorData
Viz->>Reader: assemble(proc_data)
Reader-->>Viz: AssembledData
Viz->>Renderer: render_1d/2d/3d_slice(frame)
Renderer->>FS: write PNG
alt mp4 enabled
loop each step
Viz->>Reader: read step
Reader-->>Viz: AssembledData
Viz->>Renderer: render frame
Renderer->>FS: save frames
end
Renderer->>FS: encode MP4 (imageio)
end
Viz-->>User: report saved outputs
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Updates visualization.md with comprehensive CLI viz docs covering basic usage, timestep selection, rendering options, 3D slicing, video generation, and format selection. Adds viz references to getting-started, running, case, and troubleshooting pages. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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.
- Extract shared assembly helper (assemble_from_proc_data) to deduplicate multi-processor assembly logic between reader.py and silo_reader.py - Remove dead code: unused read_record() and _read_silo_object() - Remove duplicate discover_timesteps_silo (reader.discover_timesteps handles both) - Fix renderer.py: use try/finally for writer.close(), report missing imageio - Fix viz.py: validate single-int --step against available timesteps, report error when MP4 generation fails - Fix silo_reader.py: defensive check for "silo" attribute, clarify Fortran-order reinterpretation assumption in comment - Document m/n/p convention difference in ProcessorData docstring - Pin lower-bound versions for imageio>=2.33, imageio-ffmpeg>=0.5.0 - Use integer arithmetic for precision auto-detection - Add warnings for skipped processor files during assembly Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
ⓘ Your monthly quota for Qodo has expired. Upgrade your plan ⓘ Paying users. Check that your Qodo account is linked with this Git user account |
5 similar comments
Contributor
ⓘ Your monthly quota for Qodo has expired. Upgrade your plan ⓘ Paying users. Check that your Qodo account is linked with this Git user account |
Contributor
ⓘ Your monthly quota for Qodo has expired. Upgrade your plan ⓘ Paying users. Check that your Qodo account is linked with this Git user account |
Contributor
ⓘ Your monthly quota for Qodo has expired. Upgrade your plan ⓘ Paying users. Check that your Qodo account is linked with this Git user account |
Contributor
ⓘ Your monthly quota for Qodo has expired. Upgrade your plan ⓘ Paying users. Check that your Qodo account is linked with this Git user account |
Contributor
ⓘ Your monthly quota for Qodo has expired. Upgrade your plan ⓘ Paying users. Check that your Qodo account is linked with this Git user account |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
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.
Contributor
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 |
Contributor
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.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
bot
added
the
size:XXL
Contributor
Nitpicks 🔍
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Contributor
|
CodeAnt AI finished reviewing your PR. |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
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.
- Validate Fortran record length is non-negative - Clip searchsorted indices to prevent out-of-bounds in assembly - Check silo_hdf5 directory exists before listing - Catch discover_format FileNotFoundError for clean CLI output Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
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.
…acklevel Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
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.
- Copy opts dict in render_mp4 to avoid mutating caller's dict - Only delete generated frame files during cleanup, not all files - Add missing-file warning in silo reader (consistent with binary reader) - Fix render_mp4 comment and _resolve_path docstring accuracy Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
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.05% 44.05%
=======================================
Files 70 70
Lines 20496 20496
Branches 1989 1989
=======================================
Hits 9030 9030
Misses 10328 10328
Partials 1138 1138 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Validate Fortran trailing record markers to detect file corruption - Add ndim else clauses to catch unsupported dimensionality - Narrow broad except Exception to specific types in MP4 writer - Exit with code 1 on MP4 generation failure - Clean stale frames from _frames/ before rendering - Validate --format argument against supported formats - Respect log-scale in MP4 auto-range sampling - Validate slice_axis parameter in render_3d_slice - Show available timestep range on --step mismatch - Handle per-step exceptions in PNG rendering loop - Improve docstrings for ProcessorData, _detect_endianness, render_mp4 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
bot
added
size:XXL
Contributor
|
CodeAnt AI Incremental review completed. |
- Add np.isfinite() checks to LogNorm guards in both 2D and 3D renderers so all-NaN data doesn't crash matplotlib - Wrap stale-frame and post-encode cleanup in try/except OSError so locked or missing files don't abort rendering Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
bot
added
size:XXL
Contributor
|
CodeAnt AI Incremental review completed. |
Claude Code ReviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
Claude Code ReviewNo issues found. Checked for bugs and CLAUDE.md compliance. Scope reviewed: Items verified:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Summary
./mfc.sh vizcommand for rendering post-processed output (PNG snapshots and MP4 videos) directly from the CLI — no ParaView/VisIt neededformat=2) and Silo-HDF5 (format=1) output formats with multi-processor assemblyUsage
New files
toolchain/mfc/viz/package:reader.py(binary),silo_reader.py(Silo-HDF5),renderer.py(matplotlib/imageio),viz.py(CLI entry point)commands.py,main.py,args.pyDependencies
imageio+imageio-ffmpegadded topyproject.tomlfor portable MP4 encoding (bundles ffmpeg binary)h5pyoptional for Silo-HDF5 readingTest 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--list-varsand--list-stepsinfo commandsmfc.vizimports still work viaviz_legacy.py🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Examples
Chores
Notes
CodeAnt-AI Description
Add a new ./mfc.sh viz command to render images and videos from post-processed output
What Changed
Impact
✅ Generate MP4 from timesteps via CLI✅ Inspect available variables and timesteps without a GUI✅ Visualize binary or Silo-HDF5 output without ParaView/VisIt💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.