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

Add an examplar of dcm_file path in seqinfo #333

Closed
wants to merge 2 commits into from

Conversation

bpinsard
Copy link
Contributor

Add an examplar of a dicom file in seqinfo to allow access more information in infotodict.

@codecov-io
Copy link

codecov-io commented Apr 17, 2019

Codecov Report

Merging #333 (1137713) into master (80a6538) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #333   +/-   ##
=======================================
  Coverage   77.64%   77.64%           
=======================================
  Files          41       41           
  Lines        3167     3167           
=======================================
  Hits         2459     2459           
  Misses        708      708           
Impacted Files Coverage Δ
heudiconv/dicoms.py 82.11% <ø> (ø)
heudiconv/utils.py 90.03% <ø> (ø)
heudiconv/parser.py 92.70% <100.00%> (ø)

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 80a6538...1137713. Read the comment docs.

@satra
Copy link
Member

satra commented Apr 18, 2019

@bpinsard - i think this is a good start. it would be nice to add a test, an example heuristic that uses the dicom info from the file.

@yarikoptic @mgxd what do you think?

more generally would it be useful to pass paths to all files in a group? obviously we can't store this in the dicominfo.tsv, but we are storing this in the filegroup.json, so could we simply pass that along with seqinfo?

@yarikoptic
Copy link
Member

I am worrying of storing full paths there, since then it would prevent any relocation etc.
This sounds like a workaround for some specific use-case of missing information in the seqinfo. So what was it @bpinsard ?

@yarikoptic
Copy link
Member

@bpinsard could you please followup with description of the problem/use-case you are trying to address here?

@bpinsard
Copy link
Contributor Author

At the acquisition center we store information in the following dicom tags:

  • ReferringPhysicianName
  • StudyDescription
  • PatientName
    Which I use to find the subject_id, as well as research group, study, to build the locator in infotoids.

And I also extract information from:

  • dcm.csa_header['tags']['PhaseEncodingDirectionPositive']['items'][0]
  • dcm.dcm_data.InPlanePhaseEncodingDirection
  • dcm.dcm_data.ImageComments
    to identify the sequence, if it is a sbref, and phase-encoding direction more reliably than from the ProtocoleName or else.

I don't know if this is relevant to other people. If not we can close that.

@yarikoptic
Copy link
Member

hold on -- we are already storing a filename and a directory for the file:

        info = SeqInfo(
            total,
            op.split(series_files[0])[1],
            series_id,
            op.basename(op.dirname(series_files[0])),

But why don't we also extend the SeqInfo record with found to be useful additional fields to contain the ones you desire so there is no need to re-load the dicom?

@bpinsard
Copy link
Contributor Author

bpinsard commented Dec 7, 2019

SeqInfo does include filename and the folder but the later is only the name of the last folder in the path that contains the dicom, not the full path. Is there a specific reason for that? If we remove the basename call, I think this leaves the opportunity to reread a single dicom per series, even if not optimal, and extract any kind of information without creating a very large SeqInfo structure.

@yarikoptic
Copy link
Member

SeqInfo does include filename and the folder but the later is only the name of the last folder in the path that contains the dicom, not the full path.

oh, right -- I was too fast in misreading that code. Thanks for the explanation ;)

Is there a specific reason for that?

probably again just to not leak absolute paths there, and provide minimal sufficient (someone might want to use filename, some name of the immediate directory with dicoms) information for the heuristic.

@yarikoptic
Copy link
Member

I believe #581 / #637 (whatever comes first) will provide the alternative solution to this which would not need to leak the entire filename in the metadata

@yarikoptic yarikoptic closed this Feb 17, 2023
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.

None yet

4 participants