-
Notifications
You must be signed in to change notification settings - Fork 20
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
Fixes and improvements (by mtnhuck) #13
Conversation
scanner_software_versions_pd get from SoftwareVersions edited at line 165: dict_append(image03_dict, 'scanner_software_versions_pd', metadata.get("HardcopyDeviceSoftwareVersion", "")) to dict_append(image03_dict, 'scanner_software_versions_pd', metadata.get("SoftwareVersions", "")) image_slice_thickness SliceThickness Inserted at line 170 dict_append(image03_dict, 'image_slice_thickness', metadata.get("SliceThickness", ""))
Edited to include image_orientation image_orientation: modified from https://stackoverflow.com/questions/34782409/understanding-dicom-image-a ttributes-to-get-axial-coronal-sagittal-cuts and rordenlab/dcm2niix#153 (comment) photomet_interpret: from DICOM/JSON field PhotometricInterpretation
Added a notes section talking about required experiment_id field
@yarikoptic coudl you have a look? |
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 have left some confirmations and questions to @mtnhuck so I could make an informed recommendation ;)
|
||
## Notes: | ||
Column experiment_id must be manually filled in for now. | ||
This is based on experiment ID's received from NDA after setting the study up through the NDA website [here](https://ndar.nih.gov/user/dashboard/collections.html). |
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.
am I reading it right @mtnhuck that it is pretty much yet another "mapping" but now for the task/runs? unfortunately that url requires a login so can't assess. Could you please give an example?
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 is a number that comes from the NDA archive, ie they provide you with a number for your experiment. I'm still waiting to hear back from them if it is at the session level or the scan level (ie would resting-state need a different experiment number than a face-perception task?). An example number which I generated on their website is 867.
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.
Ok, let's wait for them to reply and then make it an option or mapping
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.
According to the .gov ppl, experiment_id
is per "experiment within the study" so resting state would get one and face-perception would get another.
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.
As agreed later on, this is the id for user to provide. We have a preliminary patch ready to submit (will be done after this PR is merged) to provide the mapping from BIDS task id to experiment_id via a simple .tsv file.
@@ -162,11 +162,22 @@ def run(args): | |||
dict_append(image03_dict, 'image_modality', "MRI") | |||
dict_append(image03_dict, 'scanner_manufacturer_pd', metadata.get("Manufacturer", "")) | |||
dict_append(image03_dict, 'scanner_type_pd', metadata.get("ManufacturersModelName", "")) | |||
dict_append(image03_dict, 'scanner_software_versions_pd', metadata.get("HardcopyDeviceSoftwareVersion", "")) | |||
dict_append(image03_dict, 'scanner_software_versions_pd', metadata.get("SoftwareVersions", "")) |
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.
+1 -- I do not see the HardcopyDeviceSoftwareVersion present anywhere.
If they were interested in all "versions", then we could also extend it with ConversionSoftwareVersion but since they do not care -- "better" for us, worse for reproducibility ;)
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 this one is settled
bids2nda/main.py
Outdated
dict_append(image03_dict, 'magnetic_field_strength', metadata.get("MagneticFieldStrength", "")) | ||
dict_append(image03_dict, 'mri_echo_time_pd', metadata.get("EchoTime", "")) | ||
dict_append(image03_dict, 'flip_angle', metadata.get("FlipAngle", "")) | ||
dict_append(image03_dict, 'receive_coil', metadata.get("ReceiveCoilName", "")) | ||
dict_append(image03_dict, 'image_slice_thickness', metadata.get("SliceThickness", "")) |
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 be ok, strange thing they do not define units anywhere in their schema
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.
It looks like in the image03.txt they provide on their website they define acceptable attributed and mm is standard for most.
bids2nda/main.py
Outdated
dict_append(image03_dict, 'magnetic_field_strength', metadata.get("MagneticFieldStrength", "")) | ||
dict_append(image03_dict, 'mri_echo_time_pd', metadata.get("EchoTime", "")) | ||
dict_append(image03_dict, 'flip_angle', metadata.get("FlipAngle", "")) | ||
dict_append(image03_dict, 'receive_coil', metadata.get("ReceiveCoilName", "")) | ||
dict_append(image03_dict, 'image_slice_thickness', metadata.get("SliceThickness", "")) | ||
dict_append(image03_dict, 'photomet_interpret', metadata.get("PhotometricInterpretation", "")) |
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 be ok -- I think they do not define possible values so whatever we have might be as good
bids2nda/main.py
Outdated
elif plane[1] == 1: | ||
dict_append(image03_dict, 'image_orientation.', "Coronal") | ||
elif plane[2] == 1: | ||
dict_append(image03_dict, 'image_orientation.', "Axial") |
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.
ha ha ;) I wondered if that is what you guessed or was informed logic (I didn't look into what they exactly mean yet)? ;)
If "logic" is informed/correct, then it might as well be just
dict_append(
image03_dict, 'image_orientation.',
['Sagital', 'Coronal', 'Axial'][np.argmax(plane[:3])]
)
i.e. simply go after maximal one. Not sure if it could happen that either two of them >= 0.5
resulting to rounding to 1 in more than one, or none (all round to 0). Going after maximum would remove the ambiguity and make code shorter/without duplication.
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.
Also not sure if the sign couldn't be negative -- then may be also first wrap it with np.abs()
(I think it shouldn't alter the logic of defining the slicing)
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.
e.g. in your case:
sub-sid000447/ses-1/func/sub-sid000447_ses-1_task-selfvalence_run-03_bold.json: "ImageOrientationPatient": [0.99910127013954, -0.0366068325451, -0.0213680091858, 0.03960414059805, 0.98585875034027, 0.16283131279032],
sub-sid000448/ses-1/anat/sub-sid000448_ses-1_acq-MPRAGE_T1w.json: "ImageOrientationPatient": [-0.004293648487, 0.95983065031276, 0.28054712135906, 0.05798604985148, 0.28031661899416, -0.9581545862429],
sub-sid000448/ses-1/dwi/sub-sid000448_ses-1_acq-DTIXhardi64_dwi.json: "ImageOrientationPatient": [0.9983085405984, -0.0120986643356, 0.05686545603079, 0.00679536266945, 0.99568534650845, 0.09254465837924],
so -- was T1 coronal and the diffusion and bold Sagital? (I would think they were Axial, but I am not familiar with your protocols) Is it just a matter of swapping Axial and Sagital? ;)
@chrisfilo please add me to the "Contributors" of a kind so I could push fixes to the PR |
Done. Try now. |
bids2nda/main.py
Outdated
|
||
plane = metadata.get("ImageOrientationPatient") | ||
get_orientation = lambda place: ['Axial','Coronal','Sagittal'][np.argmax(plane[:3])] | ||
dict_append(image03_dict, 'image_orientation.',get_orientation(plane)) |
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.
after looking at this data -- I think this is not sufficient, and produces incorrect result. Let's wait for Matthew to reply on #12 (I will buzz him). FWIW here is my code which is bit more detailed but I think I am missing the point that it should be paired with affine or smth. nibabel/orientations.py actually is probably having the right function to use ;-)
def cosine_to_orientation(img_ornt_pat):
"""Deduce slicing from cosines
From http://nipy.org/nibabel/dicom/dicom_orientation.html#dicom-voxel-to
-patient-coordinate-system-mapping
From Section C.7.6.1.1.1 we see that the "positive row axis" is left to
right, and is the direction of the rows, given by the direction of last
pixel in the first row from the first pixel in that row. Similarly the
"positive column axis" is top to bottom and is the direction of the columns,
given by the direction of the last pixel in the first column from the first
pixel in that column.
Let's rephrase: the first three values of "Image Orientation Patient" are
the direction cosine for the "positive row axis". That is, they express the
direction change in (x, y, z), in the DICOM patient coordinate system
(DPCS), as you move along the row. That is, as you move from one column to
the next. That is, as the column array index changes. Similarly, the second
triplet of values of "Image Orientation Patient" (img_ornt_pat[3:] in
Python), are the direction cosine for the "positive column axis", and
express the direction you move, in the DPCS, as you move from row to row,
and therefore as the row index changes.
Parameters
----------
img_ornt_pat: list of float
Values of the ImageOrientationPatient field
Returns
-------
{'Axial', 'Coronal', 'Sagital'}
"""
# we do not care about the signs
img_ornt_pat = np.abs(img_ornt_pat)
# we need to figure out first leading dimension and the 2nd
d1 = int(np.argmax(img_ornt_pat[:3]))
d2 = int(np.argmax(img_ornt_pat[3:]))
ds = tuple(sorted([d1, d2]))
try:
return {
(0, 1): 'Axial',
(1, 2): 'Sagital',
(0, 2): 'Coronal'
}[ds]
except KeyError:
raise RuntimeError(
"Could not deduce the image orientation of %r with both directions "
"being detected as %r. Give us a use-case to fine tune"
% (img_ornt_pat, ds)
)
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.
As I'm sure y'all know, the affine from the DICOM tells you the orientation of the image acquisition relative to the patient.
You get the affine from the ImageOrientationPatient, which defines two columns, and your interpretation of the third column. There are two options for the third column (assuming it's orthogonal to the other two) - the cross product of the two columns you have, or the negative cross product. If you have the per-slice positions, you can work out which of these applies - when you don't (for 3D images or greater), you've either got to fish these out of the private fields (Siemens Mosaic) or know what you're doing with the multislice DICOM format (I don't know that very well).
Once you've got the affine, you can work out the orientation, with the nibabel orientations code.
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.
Thank you @matthew-brett!
Unfortunately I am not grasping the full complexity of the situation ATM and for now will just improve this PR with my logic.
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 am taking that back! I found that my commit message said "(not correct)" so I guess I would indeed need to get a better grasp of the situation ;-) Will push another tentative solution in a second, which might be a better start (if not a finish ;)).
tried to push into @mtnhuck 's branch but permissions denied. do not see checkmark to be able to accept pushes from collaborators -- do you see it @chrisfilo here is my branch to be merged with few tuneups https://github.com/yarikoptic/BIDS2NDA/tree/enh-mtnhuck2 - I "removed" assignment of the slice_orientation for now since IMHO it is not fully correct yet (#12) |
I elevated your permissions further. Could you try again? (I don't see the checkmark, but that's because I made the PR) |
Updated for SliceThickness and PhotometricInterpretation
make fixes
Hi all, just wondering what's going on with this PR. |
I believe that experiment_id is the experiment ID from your NIH dashboard.
You group of studies, say for a particular grant, would have a collection
ID and the experiment ID would from a specified set of subjects who
performed a certain sequence of tasks during a scan session.
…On Wed, Aug 22, 2018 at 4:14 PM Yaroslav Halchenko ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In README.md
<#13 (comment)>:
> @@ -32,3 +32,7 @@ The is the file format produced by the GUID Tool: one line per subject in the fo
## Example outputs
See [/examples](/examples)
+
+## Notes:
+Column experiment_id must be manually filled in for now.
+This is based on experiment ID's received from NDA after setting the study up through the NDA website [here](https://ndar.nih.gov/user/dashboard/collections.html).
According to the .gov ppl, experiment_id is per "experiment within the
study" so resting state would get one and face-perception would get another.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#13 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AGhtiDdtR8yVXKWlXzNgwtUizyJa6RhAks5uTbu6gaJpZM4RdEND>
.
--
Jeremy Huckins
Department of Psychological & Brain Sciences
Dartmouth College
|
@@ -194,6 +200,8 @@ def run(args): | |||
dict_append(image03_dict, 'image_resolution1', nii.header.get_zooms()[0]) | |||
dict_append(image03_dict, 'image_resolution2', nii.header.get_zooms()[1]) | |||
dict_append(image03_dict, 'image_resolution3', nii.header.get_zooms()[2]) | |||
dict_append(image03_dict, 'image_slice_thickness', nii.header.get_zooms()[2]) | |||
dict_append(image03_dict, 'photomet_interpret', metadata.get("global",{}).get("const",{}).get("PhotometricInterpretation","")) |
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 beast I am not sure even where to get from. In our use-case we just adjusted the code to provide 'NA'
as a default value.
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 pinged the NIH help desk for some guidance on the interpretation of this field. Will report back.
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.
Thanks!
…consts section apparently it is not fully flattened by/after heudiconv
…2niix if available
* gh-mtnhuck/master: fix photomet and image_orientation additions Finally got it to pull PhotometricInterpretation but doesn't work for fieldmaps Update references Conflicts: bids2nda/main.py - melded together definition for image_slice_thickness so it picks it up from metadata field first and if not available, then via nibabel
Codecov Report
@@ Coverage Diff @@
## master #13 +/- ##
=========================================
Coverage ? 14.61%
=========================================
Files ? 2
Lines ? 219
Branches ? 0
=========================================
Hits ? 32
Misses ? 187
Partials ? 0
Continue to review full report at Codecov.
|
ok, pushed some changes. couldn't stop so even enabled testing so my two lines of tests do not go untested ;) After we find some reasonably small dataset to test on, we could establish some crude smoke and even functionality testing (in subsequent PRs) |
ok, I think the world will not become a worse place (if not better) with these changes, so I will go ahead and merge |
No description provided.