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

ENH: Support multi-echo EPI phase data #373

Merged
merged 2 commits into from
Oct 8, 2019

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Sep 18, 2019

Closes #368. This solution will likely be overwritten by a larger refactor of how multi-volume data are handled, but in the meantime this should allow heudiconv to convert complex fMRI data.

@yarikoptic
Copy link
Member

2.7 tests failure is probably unrelated, filed separately: #374

I will publish your phantom data (thanks again) to datalad as soon (I hope ;)) as I get access to it, so some rudimentary regression testing could be added. Especially in the light of possible future RFing we do not want to loose this particular functionality, do we? ;)

@yarikoptic
Copy link
Member

I am uploading actual data files to http://datasets.datalad.org/?dir=/dicoms/salo/MEEPI ATM. It is indeed quite huge. I wonder if we could just just a select subset of dicom files for a minimalistic test?

@effigies
Copy link
Member

Just as a side note, if you're collecting lots of DICOMs over here, they might be useful to stress test nibabel.nicom.

@yarikoptic
Copy link
Member

well, I am collecting some. There is more in dcm2niix repos: see dcm_qa* submodules of https://github.com/rordenlab/dcm2niix/
Of cause I would welcome (re)use of any of those for testing! Let me know if anything I could do to facilitate that even more ;)

@tsalo
Copy link
Member Author

tsalo commented Sep 19, 2019

@yarikoptic 612_Estimation_task1_6ET_MB4_SBRef could be removed (although it's not big) and volumes from phase/mag run could be removed, as long as the number of volumes was kept the same between 613_Estimation_task1_6ET_MB4 and 614_Estimation_task1_6ET_MB4. Would you like me to cut them down to like 50 volumes each?

@effigies
Copy link
Member

@yarikoptic I think the main thing would be to try to go through them systematically and see if nibabel interprets them correctly, which is going to require either good documentation of each or an experienced eye. For the ones that don't, we can pull them down by datalad or any other method for an extended test, and start fixing them.

But I can stop spamming your thread. I'll open an issue on nibabel.

@yarikoptic
Copy link
Member

@tsalo we could remove or just selectively get ones for testing and specify to heudiconv to process

@codecov
Copy link

codecov bot commented Oct 3, 2019

Codecov Report

Merging #373 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #373      +/-   ##
==========================================
+ Coverage   74.24%   74.28%   +0.04%     
==========================================
  Files          35       35              
  Lines        2714     2707       -7     
==========================================
- Hits         2015     2011       -4     
+ Misses        699      696       -3
Impacted Files Coverage Δ
heudiconv/convert.py 78.92% <100%> (ø) ⬆️
heudiconv/utils.py 90.04% <0%> (-0.46%) ⬇️
heudiconv/info.py 100% <0%> (ø) ⬆️
heudiconv/heuristics/reproin.py 82.5% <0%> (+0.27%) ⬆️
heudiconv/cli/run.py 77.56% <0%> (+1.02%) ⬆️

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 d31d19d...0630fa7. Read the comment docs.

@tsalo
Copy link
Member Author

tsalo commented Oct 3, 2019

@yarikoptic Sorry for the delay. test_multiecho seems like a good base for me to adapt. I have no clue how to select only a subset of dicoms for conversion though.

@yarikoptic
Copy link
Member

I guess I will just get this one merged as is, without testing for now... hands are too full, and progress must not stop! ;)

@yarikoptic yarikoptic merged commit 202f943 into nipy:master Oct 8, 2019
@kimsin98 kimsin98 mentioned this pull request Oct 24, 2019
2 tasks
@yarikoptic yarikoptic added this to the 0.6 milestone Nov 22, 2019
yarikoptic added a commit that referenced this pull request Dec 16, 2019
This is largely a bug fix.  Metadata and order of `_key-value` fields in BIDS
could change from the result of converting using previous versions, thus minor
version boost.
14 people contributed to this release -- thanks
[everyone](https://github.com/nipy/heudiconv/graphs/contributors)!

 Enhancement

- Use [etelemetry](https://pypi.org/project/etelemetry) to inform about most
  recent available version of heudiconv. Please set `NO_ET` environment variable
  if you want to disable it ([#369][])
- BIDS:
  - `--bids` flag became an option. It can (optionally) accept `notop` value
    to avoid creation of top level files (`CHANGES`, `dataset_description.json`,
    etc) as a workaround during parallel execution to avoid race conditions etc.
    ([#344][])
  - Generate basic `.json` files with descriptions of the fields for
    `participants.tsv` and `_scans.tsv` files ([#376][])
  - Use `filelock` while writing top level files. Use
    `HEUDICONV_FILELOCK_TIMEOUT` environment to change the default timeout value
    ([#348][])
  - `_PDT2` was added as a suffix for multi-echo (really "multi-modal")
    sequences ([#345][])
- Calls to `dcm2niix` would include full output path to make it easier to
  discern in the logs what file it is working on ([#351][])
- With recent [datalad]() (>= 0.10), created DataLad dataset will use
  `--fake-dates` functionality of DataLad to not leak data conversion dates,
  which might be close to actual data acquisition/patient visit ([#352][])
- Support multi-echo EPI `_phase` data ([#373][] fixes [#368][])
- Log location of a bad .json file to ease troubleshooting ([#379][])
- Add basic pypi classifiers for the package ([#380][])

 Fixed
- Sorting `_scans.tsv` files lacking valid dates field should not cause a crash
  ([#337][])
- Multi-echo files detection based number of echos ([#339][])
- BIDS
  - Use `EchoTimes` from the associated multi-echo files if `EchoNumber` tag is
    missing ([#366][] fixes [#347][])
  - Tolerate empty ContentTime and/or ContentDate in DICOMs ([#372][]) and place
    "n/a" if value is missing ([#390][])
  - Do not crash and store original .json file is "JSON pretification" fails
    ([#342][])
- ReproIn heuristic
  - tollerate WIP prefix on Philips scanners ([#343][])
  - allow for use of `(...)` instead of `{...}` since `{}` are not allowed
    ([#343][])
  - Support pipolar fieldmaps by providing them with `_epi` not `_magnitude`.
    "Loose" BIDS `_key-value` pairs might come now after `_dir-` even if they
    came first before ([#358][] fixes [#357][])
- All heuristics saved under `.heudiconv/` under `heuristic.py` name, to avoid
  discrepancy during reconversion ([#354][] fixes [#353][])
- Do not crash (with TypeError) while trying to sort absent file list ([#360][])
- heudiconv requires nipype >= 1.0.0 ([#364][]) and blacklists `1.2.[12]` ([#375][])

* tag 'v0.6.0': (60 commits)
  Version boost to 0.6.0
  DOC: populate detailed changelog for 0.6.0 and tune up formatting in previous one
  Fix miscellaneous typos in ReproIn heuristic file.
  BF: fix check for the sbatch (SLURM) not being available
  ENH: make test-compare-two-versions take any two worktrees, and just show diff if results already known
  Update heudiconv/convert.py
  apply @mgxd 's suggestions, adding a warning and a timeout environment variable
  need str typecast
  Use empty string not None
  Empty acq_time results in empty cell not 'n/a'
  DOC: Clarify tarball session handling
  remove repetitive import statement
  respond to review - add explicit py2 check - change file saving strategy - use logger instead of print
  fix remaning py2 errors
  MNT: Add Python support metadata to package
  fix some python2/3 incompatibilities
  add return data (accidently removed return)
  make content unicode
  test that load_json provides filename if invalid
  explicitly name invalid json
  ...
yarikoptic added a commit that referenced this pull request Dec 16, 2019
[0.6.0] - 2019-12-16

This is largely a bug fix.  Metadata and order of `_key-value` fields in BIDS
could change from the result of converting using previous versions, thus minor
version boost.
14 people contributed to this release -- thanks
[everyone](https://github.com/nipy/heudiconv/graphs/contributors)!

Enhancement

- Use [etelemetry](https://pypi.org/project/etelemetry) to inform about most
  recent available version of heudiconv. Please set `NO_ET` environment variable
  if you want to disable it ([#369][])
- BIDS:
  - `--bids` flag became an option. It can (optionally) accept `notop` value
    to avoid creation of top level files (`CHANGES`, `dataset_description.json`,
    etc) as a workaround during parallel execution to avoid race conditions etc.
    ([#344][])
  - Generate basic `.json` files with descriptions of the fields for
    `participants.tsv` and `_scans.tsv` files ([#376][])
  - Use `filelock` while writing top level files. Use
    `HEUDICONV_FILELOCK_TIMEOUT` environment to change the default timeout value
    ([#348][])
  - `_PDT2` was added as a suffix for multi-echo (really "multi-modal")
    sequences ([#345][])
- Calls to `dcm2niix` would include full output path to make it easier to
  discern in the logs what file it is working on ([#351][])
- With recent [datalad]() (>= 0.10), created DataLad dataset will use
  `--fake-dates` functionality of DataLad to not leak data conversion dates,
  which might be close to actual data acquisition/patient visit ([#352][])
- Support multi-echo EPI `_phase` data ([#373][] fixes [#368][])
- Log location of a bad .json file to ease troubleshooting ([#379][])
- Add basic pypi classifiers for the package ([#380][])

Fixed

- Sorting `_scans.tsv` files lacking valid dates field should not cause a crash
  ([#337][])
- Multi-echo files detection based number of echos ([#339][])
- BIDS
  - Use `EchoTimes` from the associated multi-echo files if `EchoNumber` tag is
    missing ([#366][] fixes [#347][])
  - Tolerate empty ContentTime and/or ContentDate in DICOMs ([#372][]) and place
    "n/a" if value is missing ([#390][])
  - Do not crash and store original .json file is "JSON pretification" fails
    ([#342][])
- ReproIn heuristic
  - tolerate WIP prefix on Philips scanners ([#343][])
  - allow for use of `(...)` instead of `{...}` since `{}` are not allowed
    ([#343][])
  - Support pipolar fieldmaps by providing them with `_epi` not `_magnitude`.
    "Loose" BIDS `_key-value` pairs might come now after `_dir-` even if they
    came first before ([#358][] fixes [#357][])
- All heuristics saved under `.heudiconv/` under `heuristic.py` name, to avoid
  discrepancy during reconversion ([#354][] fixes [#353][])
- Do not crash (with TypeError) while trying to sort absent file list ([#360][])
- heudiconv requires nipype >= 1.0.0 ([#364][]) and blacklists `1.2.[12]` ([#375][])

* tag 'v0.6.0':
  Boost perspective release date in changelog to today
  ENH(TST): Fix version to older pytest to ease backward compatibility testing
  RF: use tmpdir not tmp_path fixture
  FIX: minor typo in CHANGELOG.md
@tsalo tsalo deleted the support-multiecho-phase branch January 30, 2020 16:49
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.

Support for complex multi-echo fMRI
3 participants