Fix NIDM append discarding all but one stats file; align with PyNIDM 4.5.0 + per-subject NIDM input - #9
Open
yibeichan wants to merge 2 commits into
Conversation
- requirements.txt: bump pynidm 4.2.4 -> 4.5.0; add explicit oxrdflib~=0.5.0 (oxigraph-backed rdflib store, pulled by pynidm 4.5.0) and prov[graph] (prov 3.0.0 moved NetworkX graph interop to an optional extra that nidm.experiment imports, which otherwise breaks NIDM conversion). - Dockerfile / Singularity: cap the forced rdflib upgrade at >=7.0.0,<8 so it stays compatible with oxrdflib (which requires rdflib<8). - run.py: look for per-subject NIDM input (sub-<label>/nidm.ttl, with session variants) ahead of the legacy top-level nidm.ttl fallback, so the app can append to newer per-subject NIDM datasets (e.g. nidm_4.5.0). Backward compatible with single-file NIDM inputs; absent subject -> no false match. Validated: 4.5.0 SIF builds and its converter imports resolve in-container; a Caltech BABS run (nidm_4.5.0 input, babs main w/ PR#385) clears input setup and starts recon-all. recon-all completion + NIDM append pending. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
The append path passed -j to fs_to_nidm alongside -n. In fs_to_nidm's append branch the per-stats-file loop accumulates by re-reading the -n file each iteration and serializing back to it; with -j the result is written to "<nidm_file>.json" instead, so every iteration restarts from the pristine -n file and only the last-globbed stats file survives. recon-all produces 21 stats files. Only one reached NIDM. On ABIDE Caltech sub-0051456 the shipped output carried 135 measurements from a single source (rh.BA_exvivo.thresh.stats); aseg.stats -- all subcortical volumes, hippocampus/amygdala/thalamus/ventricles -- plus every aparc parcellation were silently absent. Because glob order is filesystem-dependent, which single file survived also varied by node: a local repro of the same command kept mri_segstats instead, so the output was nondeterministic. Verified against PyNIDM 4.5.0 + rdflib 7.6.0 on the real subject: -j (as shipped) 1 stats source 135 measurements no -j 19 stats sources 5,188 measurements The original input NIDM is still fully preserved (921/921 ground triples, subject prov:Person intact), and the FreeSurfer activity remains joined to the original graph -- it prov:used the bidsmri2nidm T1w AcquisitionObject and is attributed to the same subject. Also stop merging fs_cde.ttl into the per-subject TTL. The CDE vocabulary is required to interpret the opaque fs_* predicates, but it is static and byte-identical for every subject and run (md5 a36c6e07 across four independent outputs) and is already shipped as its own file in nidm/. Embedding it made every per-subject TTL unique and ~2.0 MB larger for no information gain. All 5,188 measurements remain resolvable against the shared file. Per-subject TTL: 2,101,827 -> 249,724 bytes. The "files after conversion" log listed the directory before the aggregated TTL was written, so the actual deliverable never appeared in job logs -- which is why this went unnoticed. Relabelled as intermediate output and added a line reporting the aggregated TTL with its size and triple count. Both regression tests were confirmed to fail with their fix reverted. Co-Authored-By: Claude Opus 5 (1M context) <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
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.
Summary
Aligns the FreeSurfer-NIDM BIDSapp with the PyNIDM 4.5.0 stack and the per-subject NIDM datasets now produced upstream (e.g.
derivatives/nidm_4.5.0), so the app runs cleanly through BABS on the current study layout.Validating that work surfaced a silent data-loss bug in the NIDM append path, fixed here: every NIDM file this app produced carried one stats file instead of 21. No such output has been merged into a shared dataset — the only run is still unmerged in a BABS output RIA — so nothing in circulation needs regenerating.
The append path passed
-jtofs_to_nidmalongside-n. Infs_to_nidm's append branch, the per-stats-file loop accumulates by re-reading the-nfile each iteration and serializing back to it. With-jthe result is written to<nidm_file>.jsoninstead — so every iteration restarts from the pristine-nfile, and only the last-globbed stats file survives.recon-allproduces 21 stats files. One reached NIDM.On ABIDE Caltech
sub-0051456the output carried 135 measurements from a single source (rh.BA_exvivo.thresh.stats). Absent:aseg.stats— all subcortical volumes, hippocampus/amygdala/thalamus/ventricles — and everyaparcparcellation. Because glob order is filesystem-dependent, which file survived also varied by node: a local repro of the identical command keptmri_segstatsinstead, so the output was nondeterministic.Verified against PyNIDM 4.5.0 + rdflib 7.6.0 on the real subject:
FSStatsCollections-j(as shipped)-j(fixed)The merge itself was always sound and remains so: the input NIDM is fully preserved (921/921 ground triples), and the FreeSurfer activity
prov:usedthe originalbidsmri2nidmT1wAcquisitionObjectand is attributed to the same subjectprov:Person.This was invisible in job logs. The "files after conversion" listing enumerated the directory before the aggregated TTL was written, so the actual deliverable never appeared. It is now relabelled as intermediate output, and the aggregated TTL is logged with its size and triple count.
Shared CDE vocabulary no longer merged per subject
fs_cde.ttlis required to interpret thefs_*predicates —<collection> fs_000003 1664279.0has zero describing triples without it. But it is static and byte-identical for every subject and run (md5a36c6e07…across four independent outputs), and is already shipped as its own file innidm/. Embedding it made every per-subject TTL unique and ~2.0 MB larger for no information gain.All 5,188 measurements remain resolvable against the shared file — zero information lost. Per-subject TTL: 2,101,827 → 249,724 bytes. Because the file is byte-identical, git-annex now stores the vocabulary once per study rather than once per subject.
This matches the sibling fsl-nidm BIDSapp, which likewise ships
fsl_cde.ttlalongside rather than inside its mergednidm.ttl.Other changes
requirements.txt:pynidm4.2.4 → 4.5.0; add explicitoxrdflib~=0.5.0(oxigraph-backed rdflib store that PyNIDM 4.5.0 pulls in) andprov[graph].prov 3.0.0moved NetworkX graph interop to an optionalgraphextra thatnidm.experimentimports — without it, NIDM conversion imports fail.Dockerfile/Singularity: cap the forced rdflib upgrade at>=7.0.0,<8(oxrdflib requiresrdflib<8).src/run.py: look for per-subject NIDM input (sub-<label>/nidm.ttl, session variants first) ahead of the legacy top-levelnidm.ttlfallback. Backward compatible with single-file NIDM inputs; an absent subject yields no false match on another subject's file.src/segstats_jsonld(submodule): drop the directrdflib-jsonlddeclaration — deprecated, JSON-LD moved into rdflib in 6.0. It still arrives transitively (pynidm → funowl → rdflib-shim → rdflib-jsonld==0.6.1), butrdflib-shimexists precisely to keep it inert under rdflib ≥6, so this is hygiene and does not change the installed set. Also synced the stalepynidm==4.2.4pin, which was inert (the build never reads that file) but misleading.Context
dataset_description.jsonfetch; auto-resolves BIDS-inherited metadata). With older BABS the job fails at input setup on inputs lacking a top-leveldataset_description.json.Validation
segstats_jsonld.fs_to_nidm+nidm.experiment.*) imports resolve.nidm_4.5.0(babsmain) completed both subjects (19475545_1/2, exit0:0) and pushed results to the output RIA.-o(no existing NIDM) branch was checked and is not affected — it writes one output per stats file, so nothing is overwritten.test_append_command_does_not_pass_jsonldtest_shared_cde_not_merged_into_per_subject_ttl⏳ Still pending before merge
The fix is verified by direct
fs_to_nidmreproduction, not yet through a rebuilt container. Remaining:asegsubcortical volumes present).Open question
Output layout still follows CLAUDE.md /
ants_bidsapp(nidm/sub-<id>.ttl) rather thanfsl-nidm's per-subjectnidm.ttl+nidm_orig.ttl. The two sibling apps disagree; worth settling for cross-app interop, but out of scope here.🤖 Generated with Claude Code