-
Notifications
You must be signed in to change notification settings - Fork 13
Add file collection metadata as array-type fields to JSONs #445
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
Conversation
| out_metadata["FileCollection"] = [get_bidsuri(f.path, layout.root) for f in files] | ||
|
|
||
| files_metadata = [f.get_metadata() for f in files] | ||
| assert all(bool(meta) for meta in files_metadata), files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Raises an error if no metadata are available for any of the files.
| file_collection_entities = { | ||
| "echo": "EchoTime", | ||
| "part": None, | ||
| "mt": "MTState", | ||
| "inv": "InversionTime", | ||
| "flip": "FlipAngle", | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This relies on a hardcoded list of entities that indicate file collections and their corresponding metadata fields. It also does not work for file collections that are encoded with the acq entity or different suffixes, like TB1AFI (which differentiates files with acq-tr1/acq-tr2), MP2RAGE (which has both _MP2RAGE and _UNIT1 images from the same scan), or phase-difference field maps (which have suffixes like magnitude1, magnitude2, phasediff, phase1, and phase2).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should dir be included?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, you need to have separate acquisitions to get different phase encoding directions
singlesp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed the changes and they look really good to me.
tien-tong
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
I don't see add_file_collections in the API section in RTD https://cubids--445.org.readthedocs.build/en/445/api.html -- does it have to be, because it's already in the CLI? But it's weird that the API seemed to not be updated automatically.
Also, the issue describes this function well #403. I think this should be added to the RTD.
|
One thing I just noticed- I make a point that |
We don't need to index metadata in the BIDSLayout, but the drawback is that inheritance is ignored.
|
I've updated the relevant documentation (docstrings and this PR summary), so I'll merge once CI passes. |
|
Oh wait sorry I missed the thing about the API docs. I'll fix that before I merge. |
|
Okay added. |
Closes #403.
In order to make this work without relying entirely on string replacement to find metadata, we need to index metadata in the BIDSLayout. Not sure how much that slows things down on large datasets with current versions of pybids.I worked around this by loading sidecars directly, but it means that it won't work with inheritance (which I think is true of CuBIDS in general, unfortunately).Changes proposed in this pull request
cubids add-file-collections, to add file collection metadata to files in dataset.echo,flip, etc. The functions then look up the corresponding metadata (EchoTimeforecho) and build a list of values across the collection (e.g.,EchoTimes = [0.01, 0.02, 0.03]), which get added to the metadata for all of the files in the collection.