-
Notifications
You must be signed in to change notification settings - Fork 169
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
[FIX] Data formats clarification #1720
base: master
Are you sure you want to change the base?
[FIX] Data formats clarification #1720
Conversation
Added explicit mentioning of:
I'm not sure that this is actual requirements from BIDS, can you confirm or dis-confirm? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1720 +/- ##
=======================================
Coverage 87.93% 87.93%
=======================================
Files 16 16
Lines 1351 1351
=======================================
Hits 1188 1188
Misses 163 163 ☔ View full report in Codecov by Sentry. |
@@ -409,20 +409,22 @@ datasets and non-compliant derivatives. | |||
|
|||
### Imaging 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.
Wondering if this heading should not be renamed now that this section has become more generic.
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 used Imaging files as generic file, including for example EEG. I can't find a good name, data file will also incompass the tabular files. But I'm open for suggestions.
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.
yeah I was more making a note also for other reviewers
of Nibabel package. | ||
|
||
Due to the important size of MRI imaging data, we RECOMMEND using compressed NIfTI files | ||
by [gzip](https://www.gzip.org/) algorithm (.nii.gz). |
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.
by [gzip](https://www.gzip.org/) algorithm (.nii.gz). | |
by [gzip](https://www.gzip.org/) algorithm. |
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.
Here I wanted to stress that compressed files should conserve their original extension and have .gz
additional extension (in case if gzip can support several ones). But if it obvious, I agree with changes.
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.
Good point.
Pinging @effigies and @sappelhoff to get second opinions.
Can briefly talk about this at the maintainers meeting tomorrow.
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 think it's fine to mention .nii.gz
in brackets here.
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.
If you have the energy, implementing our soft rules would be nice:
- mostly starting every new sentence on a new line
@Remi-Gau , did you got news from maintainers meeting? Should I go with PET format too? |
Added section for PET. Essentially a copy-paste of MRI, with a minimum-recommended version of dcm2niix, and additional sub-sections to better fit into structure of page. I'll put specific remarks into a review of code. |
|
||
All PET imaging data MUST be stored using the | ||
[NIfTI-1 or NIFTI-2](https://nifti.nimh.nih.gov/) file format (`.nii`). | ||
The ANALYZE-7.5 file format (`.hdr`/`.img`), despite being very similar to NIFTI-1, |
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.
Not sure if it's still needed. Is .hdr/.img
popular in PET?
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 idea but I am starting to think we should NOT mention things that are NOT allowed, because it could get very long.
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.
@mnoergaard might know :-)
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.
For MRI is needed -- just saying NIFTI includes the .hdr/.img
. And I know at least one attempt to bidsify such format.
In addition to the imaging data (`*.nii`) a `_pet.json` sidecar file MUST be provided. | ||
The included metadata are divided into sections described below. | ||
--> |
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.
Commented this paragraph, as it appears in incorrect section (JSON must be present no matter if it's pure PET of PET-MRI dataset). Also, the same information appears directly below.
We RECOMMEND that the imaging file will be accompanied with a additional meta | ||
information extracted from source image and/or external sources in a sidecar | ||
JSON file. |
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.
For certain file the sidecar is REQUIRED so I would actually remove this sentence.
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 would agree if JSON sidecars was properly introduced in common principles. It is mentioned in several places, but without a proper definition. This paragraph tried to provide such definition.
Dynamic (multi-volume) PET imaging data SHOULD be stored in 4D, | ||
in chronological order (the order they were acquired in). | ||
|
||
Due to the important size of PET imaging data, we RECOMMEND using compressed |
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.
we do not actually officialy recommend (meaning that the validator would throw a warning if people use .nii
and not .nii.gz
)
Maybe "we suggest" or "we enourage".
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.
Recommendation was carried from common principles:
We RECOMMEND using compressed NIfTI files
volumes of functional MRI) MUST be concatenated into single image (3D or 4D, | ||
respectively). | ||
|
||
Due to the important size of MRI imaging data, we RECOMMEND using compressed |
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.
See comment about PET and recommending zipped data.
Hi @Remi-Gau , just to know is there any developments on this merge request? |
Hi @Remi-Gau, just wanted to tell that I will be away until 29 of July. |
Changes:
In common principles/image format, removed reference to Nifti, and removed Nifti specific content. Rephrased the rest.
In MRI, added Data format section with Nifti requirements.
To do: