-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add bep-005 functionality, perf/asl support #126
Comments
@remi are you able to easily merge the related PR? (In our Github workflow we keep conversations in the issue rather than in the PR, for easier keeping track). You might want to cherry-pick as I did some PR-merges from your main branch to my feature-branch. There could be 1 conflict in the Concerning the testing, would it be a good idea to have a JSON containing the BIDS structure that See below a copy of the output from the 5 asl bids-examples: BIDS = bids.layout('/Users/henk/ExploreASL/bids-examples/ASL_GE_PCASL_3Dspiral');
ans = struct with fields:
BIDS = bids.layout('/Users/henk/ExploreASL/bids-examples/ASL_Philips_PCASL_2DEPI');
ans = struct with fields:
ans = struct with fields:
BIDS = bids.layout('/Users/henk/ExploreASL/bids-examples/ASL_Siemens_PASL_multiTI');Warning: Caution when using control as M0, background suppression was applied
ans = struct with fields:
BIDS = bids.layout('/Users/henk/ExploreASL/bids-examples/ASL_Siemens_PCASL_multiPLD_pepolar');
ans = struct with fields:
ans = struct with fields:
BIDS = bids.layout('/Users/henk/ExploreASL/bids-examples/ASL_Siemens_PCASL_singleTI_3DGRASE');Warning: Missing: perf/sub-Sub103_m0scan.nii
ans = struct with fields:
ans = struct with fields:
|
I agree that the test code base needs improvement because it is becoming hard to read. Yes having JSON to keep track the expected output of some test is an option: at the moment most of the testing of the behavior is done via test on what is returned by |
hey @HenkMutsaerts As part of the PR #124 to use the schema I am doing some refactoring and trying to make sure that the info of the layout can be accessed through the This also makes it easier to write tests because we just have to define the behavior of the Here is what this could give for ASL. %% 'asl001'
BIDS = bids.layout(fullfile(pth_bids_example, 'asl001')); All files but the .json are parsed (for all modalities not just ASL) so now context and labelling image are in the layout, so to access info about a file one would need to know its "index". For example. BIDS.subjects(1).perf(1)
ans =
struct with fields:
filename: 'sub-Sub103_asl.nii.gz'
ext: '.nii.gz'
suffix: 'asl'
sub: 'Sub103'
ses: ''
acq: ''
rec: ''
dir: ''
run: ''
meta: [1×1 struct]
dependencies: [1×1 struct] This can be quite annoying so it is easier to access its info with the query function. Also, note that am trying to avoid storing the name of the json sidecars in the layout because the metadata for a given file can be spread across several json spread through the different levels of the BIDS folder hierarchy (inheritance principle). meta = bids.query(BIDS, 'metadata', 'sub', 'Sub103', 'suffix', 'asl')
meta =
struct with fields:
Manufacturer: 'GE'
ManufacturersModelName: 'DISCOVERY_MR750'
SoftwareVersions: '24_LX_MR_Software_release:DV24.0_R02_1607.b'
MagneticFieldStrength: 3
ReceiveCoilName: '32Ch_Head'
MRAcquisitionType: '3D'
PulseSequenceType: 'spiral'
PulseSequenceDetails: 'GE 3d spiral pcasl product sequence: 24_LX_MR_Software_release'
ScanningSequence: 'RM'
SequenceVariant: 'NONE'
ScanOptions: 'EDR_GEMS_SPIRAL_GEMS'
EchoTime: 0.0105
FlipAngle: 111
RepetitionTimePreparation: 4.8860
ArterialSpinLabelingType: 'PCASL'
PostLabelingDelay: 2.0250
BackgroundSuppression: 1
M0Type: 'Included'
VascularCrushing: 0
AcquisitionVoxelSize: [3×1 double]
BackgroundSuppressionNumberPulses: 4
BackgroundSuppressionPulseTime: [4×1 double]
LabelingLocationDescription: '~8 cm below the circle of Willis, through the proximal V3 segment of the vertebral arteries'
LabelingDistance: 40
LabelingDuration: 1.4500 Associated files would be in the Here for ASL. PS: I want to improve the query for dependencies because this is not ideal yet. dependencies = bids.query(BIDS, 'dependencies', 'sub', 'Sub103', 'suffix', 'asl');
dependencies{1}.labeling_image
ans =
struct with fields:
filename: 'sub-Sub103_asllabeling.jpg'
dependencies{1}.context
ans =
struct with fields:
content: [1×1 struct]
meta: []
dependencies{1}.m0
ans =
struct with fields:
type: 'within_timeseries'
explanation: 'M0 is one or more image(s) in the *asl.nii[.gz] timeseries'
volume_index: 1
Let me know what you think and if you have suggestions. |
To start with, I noticed that in the bids validator they also mentioned sidecars other than json as separate "modalities", probably because they are parsed as well. Why not simply parsing nii (or other data format) only, and not parsing the sidebars? Perhaps in some non imaging cases a tsv can be the data itself, but this is probably not a very frequent case. I see now that you have "dependencies" for the additional sidecars. I guess it boils down to where we want to use bids-matlab for. To me, the bids validator should do the validating, so we don't have to keep the file structure intact and could use a structure that is more useful (like having all metadata under a single struct for a single nii). At least for pipelines this seems more useful to me... |
Yes in general, the idea would be to leave the json alone as much as possible and only look them up when querying for some metadata. There are a potential few exceptions: when the json is not a sidecar (ex: dataset_desciption, and a few other instances). Also some metadata might have to be loaded when parsing especially regarding the "dependency" issue (see below). I am also very tempted to only load the content of the tsv upon query (when needed and asked by the user): if you have large functional datasets with one tsv per run per subject, the loading could get a tad long and the structure a bit heavy.
Yeah the proper validation should be left to the validator, the only "validation" we do is when using the bids-schema when parsing (though user can easily ask for a schema-less parsing). And the idea to "centralize" all metadata for each file in the same structure was also recently suggested by @nbeliy and indeed it makes a lot of sense. Especially for files with an associated fmap, it was suggested that the fmap should be listed as a dependency for that file. So currently we have:
The creation of a field dependency in the layout structure is mostly to facilitate fetching those dependencies by |
See the integration of asl in the bids-specification, has been incorporated in the validator 1.6 and examples are stored as well here (called
asl001..5
).The text was updated successfully, but these errors were encountered: