Single workflow - #5
Draft
arkiev wants to merge 104 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!
- New `phantomkit plot longitudinal` command (plotting/longitudinal.py) - X-axis: date of scan, Y-axis: ADC/T1/T2, vial selector dropdown - Temperature-adjustable reference line, per-session toggle buttons - scan_date embedded in HTML by pipeline via dcm2niix JSON sidecar - scan_date threaded through phantom_processor → PhantomSessionWf → plot functions - --date CLI option for existing HTML files without embedded scan_date
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>
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.