Single workflow - #5
Draft
arkiev wants to merge 113 commits into
Draft
Conversation
… scatter, updated checkpoints, didn't crash
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try the Wiz Code extension for VS Code, JetBrains, or Visual Studio. |
…API change (plugin → worker) and Click's automatic underscore-to-hyphen conversion in option names (--metric_dir → --metric-dir).
arkiev
marked this pull request as ready for review
March 20, 2026 01:49
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5 +/- ##
===========================================
- Coverage 68.73% 20.70% -48.04%
===========================================
Files 12 26 +14
Lines 1206 5172 +3966
Branches 110 739 +629
===========================================
+ Hits 829 1071 +242
- Misses 350 4058 +3708
- Partials 27 43 +16 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…HECK TEMPLATE T1!
Brings in: T1 (TI) map SNR/CNR HTML fix, eddy --nthr auto-detection, and T1_mapping fix. Resolved conflict in scan_directory() by keeping the correct mprage_dirs/candidate_dwi checks from name_convention_test (single_workflow incorrectly referenced undefined t1_dirs). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This reverts commit d36c97f.
This reverts commit df3605f.
Without PYTHONUNBUFFERED, the child's stdout is fully block-buffered since it's a pipe rather than a tty, so print()-based progress from a long-running pipeline run never reached the browser until the process exited — indistinguishable from a permanent hang on real data. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Previously, if a container named phantomkit-gui was already running, the script silently reconnected to it instead of restarting — so a stale container kept serving indefinitely even after rebuilding and pushing a fixed image. docker run also never pulls automatically once a tag exists locally, so an explicit pull is needed to pick up updates to a mutable tag. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Root cause of the reported "hangs at Running: /api/run/pipeline" report: the launcher only mounted $HOME, remapped to /hostuser. Data outside $HOME (e.g. an external RAID drive under /media/...) doesn't exist in the container, so mkdir on the output path walked up to the container's real filesystem root and hit PermissionError — before the pipeline subprocess was ever spawned, and before the response became a stream, so the frontend's SSE reader had nothing to show and just sat there indefinitely. - gui.py: move output_dir.mkdir() inside the streaming generator and wrap it in try/except, yielding a normal SSE error line + __done__1 on failure instead of letting FastAPI return a bare 500. - phantomkit-gui.sh/.bat: mount $HOME (and common external-drive locations: /media, /mnt, /Volumes) at their own identical path instead of remapping to /hostuser, so there's no host/container path translation to get wrong. PHANTOMKIT_EXTRA_MOUNT covers anything else. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
process_session() was being called with output_dir=output_dir (the top-level session folder) instead of the staging subfolder, so PhantomProcessor's outputs (metrics, vial_segmentations, images_template_space, etc.) landed directly in the output root. The staging dir, now containing nothing but temp NIfTIs that get deleted afterward, ended up empty and was removed entirely. Renamed native_contrasts_staging -> native_contrasts since it's not just a staging area — it's the permanent home for native-contrast (T1/IR/TE) QC outputs, same role as the per-series folder on the DWI side. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The two tabs were identical (file list + output + phantom dropdown + run button + log), differing only in which endpoint they posted to. Replaced the separate Longitudinal tab with a checkbox on Compare that switches the target endpoint between /api/run/compare and /api/run/longitudinal. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
convert_series_to_nii()'s MIF branch unconditionally requested -export_grad_fsl, but that only works for images with an actual diffusion gradient scheme in their header. Any anatomical series (e.g. MPRAGE) supplied as .mif.gz — rather than DICOM/NIfTI, which take a different code path — made mrconvert fail outright with "no gradient information found", crashing Stage 1 before it could even start. Falls back to a plain conversion (no gradient export) when the gradient-aware attempt fails. Verified against mrconvert directly: real DWI data still exports bvec/bval on the first attempt; a gradient-less anatomical image now converts successfully via the fallback instead of raising. Separately: Stage 1/3 failures were formatted via plain str() on the caught exception, which for CalledProcessError only shows the command and exit code — the actual captured stderr (the only thing that explains *why* it failed) was silently discarded. Added _format_exc() to include it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
extract_numeric() took the *last* number anywhere in the filename, which after staging includes a trailing, unrelated scan reference number (e.g. "TE_14_34": 14 is the actual echo time, 34 is a series number appended later) — so TE/TI values were silently wrong whenever that trailing number happened to be present. Now matches the number immediately following the TE/TI/IR token instead, handling both "TE_14" and no-underscore "SIM-TE83ms" naming styles, falling back to the old last-number behavior only when no such token exists (preserves existing test cases with no TE/TI marker). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
PhantomKit expects one subdirectory per series, named with tokens
(MPRAGE/TI/TE/DWI) it can classify. Some real DICOM exports dump every
series' files into one arbitrarily-nested folder with no per-series
structure at all, which scan_input_dir/scan_directory can't classify.
- pipeline.py: add _stage_input(), which falls back to running
dcm2niix (-f "%d", SeriesDescription-based naming) across the whole
input tree only when normal classification finds nothing, then
stages the result through the existing _wrap_flat_inputs(). Purely
additive — only triggers when the pipeline would otherwise find
nothing to process.
- Widen the TE/TI classification regex to also match no-separator
naming ("TE83ms", "TI1100ms"), not just "TE_83"/"TI_1100" — found
while verifying against real data from this exact bug report.
- Fix a DWI AP/PA naming collision: dcm2niix's SeriesDescription-based
naming can't distinguish forward/reverse phase-encode acquisitions
when the raw DICOM header uses the same description for both (common
in practice) — verified this breaks the existing filename-token-based
AP/PA pairing logic in dwi_processing.py entirely. Added
_relabel_dwi_pe_collisions(), which resolves the collision using each
file's actual PhaseEncodingDirection instead.
- cli.py: switch its _wrap_flat_inputs call to _stage_input (its only
other DICOM-per-subdirectory dcm2niix invocation is untouched).
- _cleanup_staged_input() also removes the new _staged_dicom/ dir.
- Add phantomkit/tests/test_pipeline.py (20 tests, no existing test
file previously covered any of this staging logic).
Verified end-to-end against the real reported dataset: dcm2niix
recursion from the top-level folder, TE/TI classification, and AP/PA
pairing (via dwi_processing.classify_candidates/match_ap_pa_pairs) all
confirmed working, not just unit-tested in isolation.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
PhantomKit classifies series entirely from human-assigned folder names (e.g. "..._TE_80" -> TE=80ms). If someone mislabels a folder, the wrong value silently flows through to plots/metrics with no warning, even though the real value is already sitting in the JSON sidecar's EchoTime/InversionTime/PhaseEncodingDirection fields — confirmed nothing in the codebase ever reads those fields today. - pipeline.py: new _validate_te_ti_header(), called from run_stage3's staging loop for every classified TI/TE series. Best-effort: silently no-ops if there's no JSON, no matching field, or no TE/TI token to compare against. ±1ms tolerance for JSON float rounding. - stage_series_dir()'s MIF branch now requests -json_export (previously never produced a sidecar at all for MIF-origin series, unlike the DICOM/existing-NIfTI branches) so the check can also cover MIF input. - dwi_processing.py: classify_candidates() now cross-checks each DWI candidate's filename-implied phase-encode direction against PhaseEncodingDirection via the already-existing detect_pe_direction()/ get_pe_from_json() helpers (previously only wired into the narrow dcm2niix-fallback path from earlier today). Classification itself is unchanged — still goes by filename — this only adds a warning. Verified end-to-end against the real WANIF dataset: zero warnings on correctly-labeled real series, and confirmed the warning fires correctly against deliberately mislabeled copies (wrong TE value, and an AP folder copied with a wrong PA-implying name). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
New "Vendor Comparison" GUI tab + phantomkit vendor-compare CLI command:
given a vendor-generated parametric map (e.g. siemens_ADC.nii.gz) and a
completed pipeline session, compute per-vial stats on the vendor image
using phantomkit's own already-registered vial masks, and produce an
HTML report comparing them against phantomkit's own already-computed
values — interactive NiiVue viewer (per-vial + toggle-all) plus a
per-vial comparison chart with Mean/Median and error-bar-variant
toggles.
Almost entirely reuses existing infrastructure rather than building new
subsystems:
- phantomkit/vendor_compare.py: locate_reference() (glob-locate the
existing report + vial masks for a session/map-type) and
compute_vendor_vial_stats() (regrid vial masks onto the vendor
image's own grid, nearest-neighbor — matching phantom_processor.py's
existing mrgrid convention — then reuse the already-generic
extract_pet_vial_metrics() from pet_processing.py for the actual
mrstats/mrdump extraction).
- phantomkit/plotting/vendor_compare_html.py: reuses
_niivue_viewer_panel(), PK_CONTROLS_HTML/PK_TOGGLE_JS/
_compute_pk_err_bounds()/ERROR_BAR_PLUGIN_JS (_html_common.py) and
compare_plots._load_embedded()/_extract() unmodified. phantomkit's
own series has no mean/median distinction (ADC: a fixed value ± std
from its own report; T1/T2: a single curve-fit value ± SE) so its
point never moves under the Mean/Median toggle — only the
freshly-computed vendor series does.
- compare_plots._extract(): small fix so the ADC/vial_intensity case
populates se_vals from the already-embedded stds field (previously
hardcoded to {}), giving ADC the same point+spread representation
T1/T2 already had.
- cli.py: new top-level `vendor-compare` command (hand-registered like
`pipeline`, not auto-discovered under `plot`, since it does real
mrgrid/mrstats computation rather than rendering pre-existing data).
- gui.py: new tab + /api/run/vendor-compare endpoint. Also fixes a
real bug found while wiring the vendor-image file picker: the
browser modal's click-to-select-a-file handler was hardcoded to
`_brMode === 'html'` only, so a new file-picker mode would have let
you navigate into folders but never actually select a file.
Verified end-to-end against real WANIF pipeline output (registration,
not synthetic fixtures): using phantomkit's own ADC.nii.gz as a
stand-in "vendor" image reproduces phantomkit's own per-vial ADC
values exactly (0.0000 diff across all 8 relevant vials), confirming
the regrid+extract path is numerically consistent with the existing
extraction pipeline. 8 new unit tests, all passing; no regressions in
the existing 62.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Convert MIF-format vendor images to NIfTI before use (phantomkit.vendor_compare.ensure_nifti). The viewer's nifti_to_base64() parses raw NIfTI-1 header byte offsets directly; MIF has a completely different (ASCII-header) layout, so handing it a MIF file silently produced garbage that hung the browser trying to render it. mrgrid/mrstats handle MIF natively, but the NIfTI conversion now happens once upfront so stats computation and the viewer stay consistent on the same file. - The viewer now only loads/shows toggle chips for vials actually being compared (the phantomkit-reference/vendor-stats intersection), not every mask in vial_segmentations/ — previously loaded irrelevant vials too (e.g. non-ADC vials when comparing an ADC map). - Re-added the calibration-config "Reference" series (red, matching vial_intensity.py/compare_plots.py's existing convention) via load_calibration_reference(), rendered as a static overlay outside the row/measure system since it has no mean/median or spread concept. Recolored the freshly-computed vendor series to yellow (#E6B800) so it no longer collides with red's established "calibration reference" meaning. phantomkit's own series stays blue. - cli.py: new --template-dir option (auto-detected if omitted, matching the plot subcommands' convention) for calibration reference lookup; --phantom's help text updated since it's no longer title-only. Verified: MIF vendor image round-trips through ensure_nifti correctly; viewer chips exclude out-of-scope vials; reference series appears with real SPIRIT calibration data and is gracefully omitted when phantom/template_dir aren't available; full CLI run with a MIF input end-to-end. 5 new tests (72 total), all passing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…error bars - ensure_nifti() now always runs mrconvert (float32, canonical strides), not just for MIF input -- nifti_to_base64() passes unusual NIfTI quirks (int16 + scl_slope, odd strides) straight to NiiVue unchanged, which could still hang the viewer even after the earlier MIF-only fix. - Add infer_adc_scale() to auto-detect and correct for the vendor ADC map's unit convention (raw mm^2/s, already x10^-3, or integer-scaled), since vendor ADC values are often reported x1000 relative to phantomkit's own display convention. - Read phantomkit's own full per-vial distribution from its xlsx (load_full_stats_from_xlsx) when available, so its ADC series gets real mean/median/error-bar behavior instead of a single fixed point -- both series now respond to the Mean/Median and error-bar toggles. - Fix a scale-conflation bug found during end-to-end testing: phantomkit's xlsx-derived stats were being scaled by the vendor's auto-detected scale instead of phantomkit's own fixed x1000 mm^2/s convention, corrupting phantomkit's values whenever the two conventions differed. Added a regression test using deliberately mismatched scales to catch this class of bug.
…on table - Vendor series is now a filled yellow circle (matching phantomkit's own filled marker style) instead of a hollow outline. - Offset phantomkit/vendor points +/-0.15 on the x-axis so the two series no longer sit exactly on top of each other when their values are close. The offset survives the Mean/Median toggle since only y is rewritten. - Add a "Per-vial comparison vs reference" table: reference value, vendor measurement, phantomkit measurement, and each series' percentage difference from the reference. Table values track the Mean/Median toggle via the existing _pkAfterUpdate hook.
Precompute the calibration reference at every temperature (not just default_temp), add a temperature dropdown above the chart, and drive both the chart's red Reference series and the comparison table's Reference/ percentage-difference columns from the selected temperature via a shared _vcSetTemp() handler. Extend the calibration-reference regression test to assert the dropdown, VC_REF_BY_TEMP payload, and table cells are present.
Compare and Vendor Comparison tabs previously offered a '(auto-detect)' phantom entry that omitted --phantom and let the CLI guess from embedded data. All three phantom dropdowns (Pipeline, Compare, Vendor Comparison) now list only real phantom names, so the user always makes an explicit choice.
…o viewer - vendor-compare now accepts repeatable --vendor-image/--vendor-label flags instead of exactly one image; the GUI's Vendor Comparison tab gets a dynamic multi-file list (reusing the Compare tab's addFileRow/ getFileRows pattern) instead of a single file field. - build_vendor_compare_html() generalizes from a fixed 2-row (phantomkit + one vendor) layout to N+1 rows: per-vial x-offsets and colors are computed for however many vendors are given, and the comparison table gains one value+delta column pair per vendor. - New _niivue_multi_bg_viewer_panel() in _html_common.py (kept separate from the existing single-background viewer to avoid any risk to the many other reports using it) lets the image viewer switch its background between every vendor image AND phantomkit's own map image. - locate_reference() now also returns phantomkit_image: the ADC.nii.gz dwi_processing.py writes alongside vial_segmentations/ (ADC only -- T1/T2 have no equivalent per-voxel map, only curve-fit CSVs), offered as an extra viewer background option. - Added image_stem() helper to vendor_compare.py (shared by ensure_nifti and default vendor-label derivation from filename).
Colored pill buttons (one per phantomkit/vendor/reference series) plus a "Toggle all" button, mirroring the Compare tab's session-toggle pattern (compare_plots.py's pkToggleSession/pkToggleAllSessions). Independent of the existing Mean/Median, error-bar, and reference-temperature controls.
The toggle buttons mutated ds.hidden directly and called chart.update(), but the shared errorBarPlugin (_html_common.py) checks chart.getDatasetMeta(idx).hidden, which only Chart.js's official setDatasetVisibility()/isDatasetVisible() API keeps in sync -- so a hidden series' error bars kept rendering. Switch to that API instead.
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
3 participants
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.
The primary contribution of this fork is the addition of a complete DWI preprocessing and QC pipeline integrated as a first-class stage alongside the existing native contrast workflow. This includes automatic DICOM series classification, phase-encoding correction mode selection (rpe_none, rpe_pair, rpe_all, rpe_split), FSL-based DWI preprocessing via dwifslpreproc, tensor fitting to produce ADC and FA maps, and rigid-body T1-to-DWI coregistration using FLIRT — all orchestrated through a new pipeline.py entry point that runs all three stages from a single command. Several refinements were also made to the existing codebase: the ANTs registration check threshold was updated, vial masks are now transformed with nearest-neighbour interpolation to preserve binary values, and the ADC scatter plot was redesigned with open-circle reference markers plotted over the measured values for clearer visual comparison against SPIRIT reference data.