#603 xASL_bids_BIDS2Legacy: Add dataset_description.json#610
#603 xASL_bids_BIDS2Legacy: Add dataset_description.json#610MichaelStritt merged 4 commits intodevelopfrom
Conversation
MichaelStritt
added
import
jan-petr
changed the title
jan-petr
left a comment
jan-petr
left a comment
There was a problem hiding this comment.
Good. But we want the same thing also in ASL.json and M0.json of derivatives - see explanation in the comments. Should be an easy fix.
Functions/xASL_bids_BIDS2Legacy.m
Outdated
| datasetDescription = spm_jsonread(fullfile(pathStudy, 'rawdata', 'dataset_description.json')); | ||
| % Add "GeneratedBy" field | ||
| datasetDescription.GeneratedBy = struct; | ||
| datasetDescription.GeneratedBy.Name = 'xASL-BIDS'; |
There was a problem hiding this comment.
Thinking if xASL-BIDS is the name we want. It might sound as conversionASL-BIDS not making the link to ExploreASL. Plain ExploreASL might be better. @HenkMutsaerts what do you think?
There was a problem hiding this comment.
Sure, changed to ExploreASL in e7ee311 👍
| % 10. Create DataPar.json | ||
| % 11. Copy participants.tsv | ||
| % 12. Clean up | ||
| % 12. Add dataset_description.json |
There was a problem hiding this comment.
Theoretically, enough to have it there. But practically, we mostly need it on the subject level - written to all ASL and M0 JSONs, because at the individual file level, we will always check what ExploreASL version was used.
Our motivation here is that we currently remove and apply the Philips scalings already when doing DCM2BIDS conversion. But we leave the SIemens and GE scalings only in the quantification module, because they are not really stored directly in DICOM. But for ASL-BIDS, we wanted to remove any scalings, independent of what is their origin. So once we implement that, we want to be sure what version of ExploreASL was used to create data - data generated by the old version will apply these scalings still in quantification, data generated by the newer version will apply the scalings already in import - as promised in ASL-BIDS.
There was a problem hiding this comment.
Sounds good. It's just added to all JSONs now in e7ee311 👍
I can remove it from the dataPar.json if you want, but it probably doesn't hurt either.
There was a problem hiding this comment.
That's an elegant way to add it. We indeed want it in all JSON data sidecars. We want it in dataset_description, it doesn't hurt in dataPar.json. We probably don't want it in participants.json as this should have fields describing columns and an extra field that doesn't match the structure can be confusing.
See the BIDS-modality-agnostic-files.html.
What do you think?
There was a problem hiding this comment.
Agree, just read about it. I exclude it in 3afe366 👍
jan-petr
left a comment
jan-petr
left a comment
There was a problem hiding this comment.
One minor discussion point.
| % 10. Create DataPar.json | ||
| % 11. Copy participants.tsv | ||
| % 12. Clean up | ||
| % 12. Add dataset_description.json |
There was a problem hiding this comment.
That's an elegant way to add it. We indeed want it in all JSON data sidecars. We want it in dataset_description, it doesn't hurt in dataPar.json. We probably don't want it in participants.json as this should have fields describing columns and an extra field that doesn't match the structure can be confusing.
See the BIDS-modality-agnostic-files.html.
What do you think?
jan-petr
force-pushed
the
feature-#603_DatasetDescription
branch
2 times, most recently
from
232809f to
3afe366
Compare
jan-petr
left a comment
jan-petr
left a comment
There was a problem hiding this comment.
OK to merge. I made a minor commit. Note that I changed the commit message so I had to remove the commit and force-pushed it again - so hopefully you didn't pull inbetween...
Anyway - extra caution during merging :)
2 participants
Linked issue
Check out #603
How to test
Just run the import workflow, you should now get a dataset_description.json within derivatives/ExploreASL
Comments
None