Skip to content
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

Merged
merged 16 commits into from
Jun 26, 2019
Merged

Conversation

chrisgorgo
Copy link
Contributor

No description provided.

mtnhuck added 3 commits January 12, 2018 17:09
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
@chrisgorgo
Copy link
Contributor Author

@yarikoptic coudl you have a look?

Copy link
Collaborator

@yarikoptic yarikoptic left a 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).
Copy link
Collaborator

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?

Copy link
Contributor

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.

Copy link
Collaborator

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

Copy link
Collaborator

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.

Copy link
Collaborator

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", ""))
Copy link
Collaborator

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 ;)

Copy link
Collaborator

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", ""))
Copy link
Collaborator

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

Copy link
Contributor

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", ""))
Copy link
Collaborator

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")
Copy link
Collaborator

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.

Copy link
Collaborator

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)

Copy link
Collaborator

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? ;)

@yarikoptic
Copy link
Collaborator

@chrisfilo please add me to the "Contributors" of a kind so I could push fixes to the PR

@chrisgorgo
Copy link
Contributor Author

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))
Copy link
Collaborator

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)
        )

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.

Copy link
Collaborator

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.

Copy link
Collaborator

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 ;)).

@yarikoptic
Copy link
Collaborator

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)

@chrisgorgo
Copy link
Contributor Author

I elevated your permissions further. Could you try again?

(I don't see the checkmark, but that's because I made the PR)

@Shotgunosine
Copy link

Shotgunosine commented Aug 22, 2018

Hi all, just wondering what's going on with this PR.

@mtnhuck
Copy link
Contributor

mtnhuck commented Aug 23, 2018 via email

@@ -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",""))
Copy link
Collaborator

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.

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.

Copy link
Collaborator

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
* 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
Copy link

codecov bot commented Jun 24, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@2b264fb). Click here to learn what that means.
The diff coverage is 60.86%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #13   +/-   ##
=========================================
  Coverage          ?   14.61%           
=========================================
  Files             ?        2           
  Lines             ?      219           
  Branches          ?        0           
=========================================
  Hits              ?       32           
  Misses            ?      187           
  Partials          ?        0
Impacted Files Coverage Δ
bids2nda/tests/test_helpers.py 100% <100%> (ø)
bids2nda/main.py 13.02% <52.63%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b264fb...735eab0. Read the comment docs.

@yarikoptic
Copy link
Collaborator

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)

@yarikoptic
Copy link
Collaborator

ok, I think the world will not become a worse place (if not better) with these changes, so I will go ahead and merge

@yarikoptic yarikoptic merged commit 300fd08 into bids-standard:master Jun 26, 2019
@ljchang ljchang mentioned this pull request Feb 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants