[ENH] Adds extraction of physio signals from DICOMs #446

pvelasco · 2020-05-05T19:23:23Z

It uses bidsphysio to extract physiological signals from CMRR EPI DICOMs.

It includes regression test

It uses [`bidsphysio`](https://github.com/cbinyu/bidsphysio) to extract physiological signals from CMRR EPI DICOMs. It includes regression test

codecov · 2020-05-05T19:55:57Z

Codecov Report

Attention: Patch coverage is 91.39785% with 8 lines in your changes missing coverage. Please review.

Project coverage is 76.96%. Comparing base (d8e5c5f) to head (15e4a49).
Report is 608 commits behind head on master.

Files	Patch %	Lines
heudiconv/convert.py	70.58%	5 Missing ⚠️
heudiconv/dicoms.py	66.66%	1 Missing ⚠️
heudiconv/heuristics/bids_PhoenixReport.py	93.33%	1 Missing ⚠️
heudiconv/heuristics/bids_physio.py	94.11%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #446      +/-   ##
==========================================
+ Coverage   76.01%   76.96%   +0.95%     
==========================================
  Files          38       40       +2     
  Lines        3010     3100      +90     
==========================================
+ Hits         2288     2386      +98     
+ Misses        722      714       -8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yarikoptic

sweet!!!!! Left notes from a very initial cursory review.

Dockerfile

heudiconv/convert.py

heudiconv/dicoms.py

heudiconv/tests/test_regression.py

heudiconv/convert.py

mgxd

this is awesome, thank you!

a few style suggestions, though we might just consider blacking the whole repo and adhering to a stricter style guide.

another consideration while looking at this - WDYT about fetching the data via datalad instead of hosting it in the repo?

heudiconv/convert.py

heudiconv/dicoms.py

yarikoptic · 2020-05-06T16:29:10Z

re blacking: I don't mind at all. But we have outstanding sizeable PRs so it would introduce conflicts... will need to check with @tsalo and @bpinsard

pvelasco · 2020-05-06T18:25:02Z

another consideration while looking at this - WDYT about fetching the data via datalad instead of hosting it in the repo?

Of course, that would be great. You can just download the data from my branch, or I can send you a zip file with them. Once they are in DataLad, I'll modify the regression tests accordingly.

... from `Dockerfile`

Co-authored-by: Mathias Goncalves <[email protected]>

tsalo · 2020-05-06T19:14:00Z

@yarikoptic, I like automated code formatting, but I think running black would introduce a ton of conflicts in #442. Could aggressive re-formatting be something that's done in its own PR after the pending larger PRs are handled?

mgxd · 2020-05-06T19:24:03Z

of course, it would make it nearly unreviewable! I meant showing some love to formatting prior to the next release, and then inserting a style check into our CI

yarikoptic · 2020-05-06T19:54:13Z

Just a note (I am ok either way): theoretically (didn't try yet) if there is no value in individual commits in a or (or if they are way to split from a full diff, eg different files), and both branches are auto blacked when pr branch having master branch merged right before blacking - should be easy to force refresh PRs with blacked version ;-) I can try if we decide to black it all

yarikoptic · 2020-05-06T19:57:45Z

Re data - I think it is time to remove the middle man me! Let's (ab)use lfs of GitHub with datalad: http://handbook.datalad.org/en/latest/basics/101-138-sharethirdparty.html?highlight=Lfs#use-github-for-sharing-content . I will initiate a repo with the content of this PR data files, and invite everyone possibly interested ;-)...

yarikoptic · 2020-05-06T19:59:53Z

Thinking about it - iirc dicoms compress nicely. I will try with enabling shared encryption - that should compress them while stored in lfs.

That reduces indentation in the function code.

To preserve backwards compatibility

…cedata This way the user can go back to the scanner and import the exact same protocol that was run for a given subject, improving reproducibility.

[`bidsphysio`](https://github.com/cbinyu/bidsphysio) has been refractored into subpackages so that now we only need to import `bidsphysio.dcm2bids`, which has fewer dependencies

Merges `adds_acq_time_to_seqinfo` into `dcm_physio` branch

Merge `handles_phoenix_file` into `dcm_physio`

The dependencies for `bidsphysio.dcm2bids>=1.4.3` require Python 3.6 or higher, so don't try to install `bidsphysio.dcm2bids` unless `python_version > 3.5`.

heudiconv/info.py

@tsalo

Thanks to @tsalo for his suggestion

yarikoptic

Sorry it took so long. Did a pass. Also of interest either bidsphysio can convert fresh WiP 1066 Siemens physio data.

edit: I will convert it to draft. Please undraft/ping whenever ready for re-review etc

yarikoptic · 2023-02-17T15:09:09Z

heudiconv/convert.py

@@ -461,6 +461,8 @@ def convert(items, converter, scaninfo_suffix, custom_callable, with_prov,
            if outtype == 'dicom':
                convert_dicom(item_dicoms, bids_options, prefix,
                              outdir, tempdirs, symlink, overwrite)
+            elif outtype == 'physio':


we would need to document that, e.g. at https://github.com/nipy/heudiconv/blob/HEAD/docs/heuristics.rst#infotodictseqinfos (make it all into a nice itemized list there for nii, dicom and now physio)

yarikoptic · 2023-02-17T15:09:45Z

heudiconv/convert.py

+            "bidsphysio.dcm2bids not found. "
+            "Not extracting physiological recordings."
+        )
+        return


I would be ok with erroring out here since if heuristic did instruct to get those physio - we better have all dependencies

yarikoptic · 2023-02-17T15:12:08Z

heudiconv/info.py

-    'datalad': ['datalad >=%s' % MIN_DATALAD_VERSION]
+    'datalad': ['datalad >=%s' % MIN_DATALAD_VERSION],
+    'physio': [
+        'bidsphysio.dcm2bids >=1.4.3; python_version>"3.5"',   # if dicoms with physio need to be converted


Suggested change

'bidsphysio.dcm2bids >=1.4.3; python_version>"3.5"', # if dicoms with physio need to be converted

'bidsphysio.dcm2bids >=1.4.3', # if dicoms with physio need to be converted

as 3.5 is already below what we support

yarikoptic · 2023-02-17T15:15:50Z

heudiconv/info.py

+    'datalad': ['datalad >=%s' % MIN_DATALAD_VERSION],
+    'physio': [
+        'bidsphysio.dcm2bids >=1.4.3; python_version>"3.5"',   # if dicoms with physio need to be converted
+    ]
 }

 # Flatten the lists


all those binary files below adding up to over 1MB -- I think should be moved to some external dataset and used via fetch_data. ATM fetch_data hardcodes to access from datasets.datalad.org but I think we should change that -- if full url is provided - just use that URL, and then share them on some repo, could be even not annex, just regular git repo on github.

Later we should improve this fetch_data to make data persistent locally etc... but not for this PR.

yarikoptic · 2023-02-17T15:17:46Z

heudiconv/utils.py

@@ -46,7 +46,8 @@
    'patient_sex',           # 23
    'date',                  # 24
    'series_uid',            # 25
- ]
+    'time',                  # 26
+]


this would conflict with WiP of #637 / #581 but the main concern is that I do not see it needed/used at all by the target need of this PR -- conversion of physio data. Spurious wishful change? ;-)

pvelasco added 2 commits May 5, 2020 15:18

[ENH] Adds extraction of physio signals from DICOMs

f4e8cfd

It uses [`bidsphysio`](https://github.com/cbinyu/bidsphysio) to extract physiological signals from CMRR EPI DICOMs. It includes regression test

Adds heuristic for physio extraction.

e832ae6

yarikoptic requested changes May 5, 2020

View reviewed changes

mgxd reviewed May 6, 2020

View reviewed changes

heudiconv/convert.py Outdated Show resolved Hide resolved

heudiconv/convert.py Outdated Show resolved Hide resolved

heudiconv/convert.py Show resolved Hide resolved

heudiconv/dicoms.py Outdated Show resolved Hide resolved

heudiconv/dicoms.py Outdated Show resolved Hide resolved

pvelasco and others added 4 commits May 6, 2020 14:29

Moves the installation of bidsphysio to info.py

aff779f

... from `Dockerfile`

Minor formating in heudiconv/convert.py

9de4bd6

Co-authored-by: Mathias Goncalves <[email protected]>

Minor formating in heudiconv/convert.py

e935490

Co-authored-by: Mathias Goncalves <[email protected]>

Minor formating in heudiconv/dicoms.py

9de5114

Co-authored-by: Mathias Goncalves <[email protected]>

pvelasco added 2 commits May 6, 2020 15:44

Minor formatting updates

892bc6a

Minor formatting updates

e2774f5

pvelasco added 2 commits May 6, 2020 16:08

Adds assert_cwd_unchanged to test_regression

cca6c7c

RF: convert - convert_physio returns if bids_options is None

532c630

That reduces indentation in the function code.

This was referenced Jun 17, 2020

Document similar software physiopy/phys2bids#251

Open

Use AcquisitionTime for acq_time column of scans tsv files #450

Closed

tsalo mentioned this pull request Aug 6, 2020

Run automated code formatter to standardize library coding style #467

Open

pvelasco added 7 commits January 6, 2021 13:35

ENH: Adds "AcquisitionTime" to the seqinfo

4ac551e

Switch back order of seqinfo_fields

5b33646

To preserve backwards compatibility

Switch back order of SeqInfo arguments in dicom.py

74d55d5

To preserve backwards compatibility

Adds unittest to check "time" in the right position in dicominfo.tsv

0fb0c2f

ENH: Allows the user to save the Phoenix Report (Siemens) in the sour…

46c0bdb

…cedata This way the user can go back to the scanner and import the exact same protocol that was run for a given subject, improving reproducibility.

Changed calls to bidsphysio to conform with newest version

371d006

[`bidsphysio`](https://github.com/cbinyu/bidsphysio) has been refractored into subpackages so that now we only need to import `bidsphysio.dcm2bids`, which has fewer dependencies

Merge branch 'master' into dcm_physio

81ec2a2

yarikoptic and others added 6 commits January 12, 2021 13:34

BF(py3.5): explicitly case path to str for open

2768e7a

ENH(minor): sort imports

fe3fc34

Merge pull request #6 from cbinyu/adds_acq_time_to_seqinfo

38d6360

Merges `adds_acq_time_to_seqinfo` into `dcm_physio` branch

Merge branch 'dcm_physio' into handles_phoenix_file

593062f

Merge pull request #7 from cbinyu/handles_phoenix_file

62841a7

Merge `handles_phoenix_file` into `dcm_physio`

Add environment marker (Py>3.5) for dcm2bids requirement.

5c78593

The dependencies for `bidsphysio.dcm2bids>=1.4.3` require Python 3.6 or higher, so don't try to install `bidsphysio.dcm2bids` unless `python_version > 3.5`.

tsalo reviewed Jan 21, 2021

View reviewed changes

heudiconv/info.py Outdated Show resolved Hide resolved

pvelasco added 2 commits January 21, 2021 16:05

Change name of extra_requires from dcm2bids to physio

0745845

Thanks to @tsalo for his suggestion

Delete some unneeded checks in the bids_physio heuristic.

15e4a49

pvelasco requested a review from yarikoptic January 22, 2021 18:03

pvelasco mentioned this pull request Nov 2, 2021

Converting physiological recordings #532

Open

1 task

yarikoptic requested changes Feb 17, 2023

View reviewed changes

yarikoptic marked this pull request as draft February 17, 2023 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] Adds extraction of physio signals from DICOMs #446

[ENH] Adds extraction of physio signals from DICOMs #446

pvelasco commented May 5, 2020

codecov bot commented May 5, 2020 •

edited

Loading

yarikoptic left a comment

mgxd left a comment

yarikoptic commented May 6, 2020

pvelasco commented May 6, 2020

tsalo commented May 6, 2020

mgxd commented May 6, 2020

yarikoptic commented May 6, 2020

yarikoptic commented May 6, 2020

yarikoptic commented May 6, 2020

yarikoptic left a comment •

edited

Loading

yarikoptic Feb 17, 2023

yarikoptic Feb 17, 2023

yarikoptic Feb 17, 2023

yarikoptic Feb 17, 2023

yarikoptic Feb 17, 2023

	'bidsphysio.dcm2bids >=1.4.3; python_version>"3.5"', # if dicoms with physio need to be converted
	'bidsphysio.dcm2bids >=1.4.3', # if dicoms with physio need to be converted

[ENH] Adds extraction of physio signals from DICOMs #446

Are you sure you want to change the base?

[ENH] Adds extraction of physio signals from DICOMs #446

Conversation

pvelasco commented May 5, 2020

codecov bot commented May 5, 2020 • edited Loading

Codecov Report

yarikoptic left a comment

Choose a reason for hiding this comment

mgxd left a comment

Choose a reason for hiding this comment

yarikoptic commented May 6, 2020

pvelasco commented May 6, 2020

tsalo commented May 6, 2020

mgxd commented May 6, 2020

yarikoptic commented May 6, 2020

yarikoptic commented May 6, 2020

yarikoptic commented May 6, 2020

yarikoptic left a comment • edited Loading

Choose a reason for hiding this comment

yarikoptic Feb 17, 2023

Choose a reason for hiding this comment

yarikoptic Feb 17, 2023

Choose a reason for hiding this comment

yarikoptic Feb 17, 2023

Choose a reason for hiding this comment

yarikoptic Feb 17, 2023

Choose a reason for hiding this comment

yarikoptic Feb 17, 2023

Choose a reason for hiding this comment

codecov bot commented May 5, 2020 •

edited

Loading

yarikoptic left a comment •

edited

Loading