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

BF: Ensure bvec/bval files are only created for dwi output #491

Merged
merged 8 commits into from
Feb 19, 2021

Conversation

pvelasco
Copy link
Contributor

@pvelasco pvelasco commented Feb 1, 2021

Currently, .bvec/.bval files are copied into the output folder whenever they are present in the dcm2niix output. dcm2niix (v1.0.20201102) creates those files whenever it detects a diffusion sequence and the images are not derived (rordenlab/dcm2niix#352 (comment)). So, with the current heudiconv, if you extract an image collected with a diffusion sequence, but with b-value = 0 as a fmap (pepolar modality), it will copy the .bvec/.bval files to the fmap folder. However, this doesn't conform to BIDS format, which only allows for .bvec/.bval for data in the dwi folder. Consequently, BIDS-validator (v1.5.10) gives an error:

bids-validator@1.5.10

	1: [ERR] Files with such naming scheme are not part of BIDS specification. This error is most commonly caused by typos in file names that make them not BIDS compatible. Please consult the specification and make sure your files are named correctly. If this is not a file naming issue (for example when including files not yet covered by the BIDS specification) you should include a ".bidsignore" file in your dataset (see https://github.com/bids-standard/bids-validator#bidsignore for details). Please note that derived (processed) data should be placed in /derivatives folder and source data (such as DICOMS or behavioural logs in proprietary formats) should be placed in the /sourcedata folder. (code: 1 - NOT_INCLUDED)
		./sub-FuncDiff/fmap/sub-FuncDiff_acq-dwi_dir-AP_epi.bval
			Evidence: sub-FuncDiff_acq-dwi_dir-AP_epi.bval
		./sub-FuncDiff/fmap/sub-FuncDiff_acq-dwi_dir-AP_epi.bvec
			Evidence: sub-FuncDiff_acq-dwi_dir-AP_epi.bvec
		./sub-FuncDiff/fmap/sub-FuncDiff_acq-dwi_dir-PA_epi.bval
			Evidence: sub-FuncDiff_acq-dwi_dir-PA_epi.bval
		./sub-FuncDiff/fmap/sub-FuncDiff_acq-dwi_dir-PA_epi.bvec
			Evidence: sub-FuncDiff_acq-dwi_dir-PA_epi.bvec

This PR makes sure we only copy the .bvec/.bval if the destination folder for an image is dwi. It also adds a unit test, and the necessary dicom file and heuristics.

`dcm2niix` produces *.bvec/*.bval files whenever it extracts an image collected with a DW sequence. However, BIDS only accepts those files in the `dwi` folders.
@pvelasco
Copy link
Contributor Author

pvelasco commented Feb 3, 2021

It fails CI because the version it installs from Neurodebian is still 1.0.20181125, while the new behavior seemed to have been introduced in version 1.0.20201102.
I'll have to investigate some more.

Copy link
Member

@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 think it would be better to be safe than sorry and check if bval is really 0. If not - issue a warning and not remove. That should also then alert bids validator, and user would need to investigate why they have those files (and why it wasn't 0 bval)

heudiconv/convert.py Outdated Show resolved Hide resolved
Specify the reason of the deletion of `.bvec` and `.bval` files for non `dwi` output.

Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
@codecov
Copy link

codecov bot commented Feb 7, 2021

Codecov Report

Merging #491 (5099d01) into master (863435e) will increase coverage by 0.34%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #491      +/-   ##
==========================================
+ Coverage   76.45%   76.79%   +0.34%     
==========================================
  Files          39       40       +1     
  Lines        3045     3090      +45     
==========================================
+ Hits         2328     2373      +45     
  Misses        717      717              
Impacted Files Coverage Δ
heudiconv/convert.py 87.13% <84.21%> (+0.63%) ⬆️
heudiconv/heuristics/test_b0dwi_for_fmap.py 92.30% <92.30%> (ø)
heudiconv/tests/test_convert.py 100.00% <100.00%> (ø)
heudiconv/utils.py 90.32% <0.00%> (+0.40%) ⬆️

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 863435e...5099d01. Read the comment docs.

@pvelasco
Copy link
Contributor Author

pvelasco commented Feb 7, 2021

I think it would be better to be safe than sorry and check if bval is really 0. If not - issue a warning and not remove.

That sounds sensible. I will work on it.

Merge nipy#492 into bvecs_just_for_dwi branch
Print out a warning if they are saved in destination folder if it is not `dwi`
Also, set a variable with the message, for future checks in unit test
Now it checks that there is a warning that `*.bvec` and `*.bval` files are present in a non-`dwi` folder
@pvelasco
Copy link
Contributor Author

@yarikoptic,
I think 9746fe6 addresses your suggestion. I have also modified the unit test accordingly.

heudiconv/convert.py Outdated Show resolved Hide resolved
@pvelasco
Copy link
Contributor Author

you saw usecases with non absolute 0 bvals where they still should be considered? just curious

No. I would not use a diffusion-weighted image (b non zero) for fmap estimation purposes, because you will always get lower SNR than the equivalent b-value = 0 image.
Note: Siemens uses all the gradients in the sequence to compute the real b-value. So even if you select b-value=0, you get a value of 5 in the header.

@yarikoptic
Copy link
Member

Great, thank you @pvelasco !

@yarikoptic yarikoptic merged commit 6734d48 into nipy:master Feb 19, 2021
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

2 participants