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] update_complex_name to handle real+imaginary complex data #761

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

bpinsard
Copy link
Contributor

Someone recently shared with me some GE Hyperband multi-echo data from a SIGNA UHP, where they exported mag + real + imaginary data (weird combination but why not), all 3 ends up in the same series. So that should ideally be handled in heudiconv within update_complex_name which only covers mag+phase case for now.

Though I hope it won't be the final export format for that project, it could make sense to cover that usecase in heudiconv.

I don't know if there are any open test data with real+imaginary data within the same series, @neurolabusc ?

@bpinsard
Copy link
Contributor Author

Codespell get called in both Codespell and Linters? It fails in both on some files that has nothing to do with that PR.

Copy link

codecov bot commented May 30, 2024

Codecov Report

Attention: Patch coverage is 62.50000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 82.42%. Comparing base (2eb5291) to head (19666a9).

Files with missing lines Patch % Lines
heudiconv/dicoms.py 16.66% 5 Missing ⚠️
heudiconv/convert.py 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #761      +/-   ##
==========================================
- Coverage   82.48%   82.42%   -0.07%     
==========================================
  Files          42       42              
  Lines        4323     4331       +8     
==========================================
+ Hits         3566     3570       +4     
- Misses        757      761       +4     

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

@bpinsard bpinsard force-pushed the enh/complex_realimag branch 2 times, most recently from c9637c0 to 464e468 Compare May 30, 2024 15:45
@bpinsard bpinsard requested a review from yarikoptic May 30, 2024 16:02
@bpinsard bpinsard force-pushed the enh/complex_realimag branch from 464e468 to 39dfd5b Compare May 30, 2024 19:38
@bpinsard
Copy link
Contributor Author

This will require rordenlab/dcm2niix#826 to work, otherwise we might need to assume that data are "MAGNITUDE" by default in update_complex_name

@yarikoptic
Copy link
Member

Codespell get called in both Codespell and Linters? It fails in both on some files that has nothing to do with that PR.

thanks! now there is

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 guess let's wait on dcm2niix decision but meanwhile provided some recommenations to changes/around... can't do more today -- better go do smth as dumb as fix a car

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

ImageType is a mess: just remembered that diffusion data on Siemens won't have that info in the ImageType, see CMRR-C2P/MB#305 for the CMRR sequence where they were able to fix that.

I wonder if dcm2niix should add a new ComplexImageComponent json tag rather than editing the ImageType to match what inconsistently exists on Siemens.

@bpinsard bpinsard force-pushed the enh/complex_realimag branch 2 times, most recently from bd3efd8 to eef6f46 Compare August 8, 2024 19:03
@bpinsard bpinsard requested a review from yarikoptic August 8, 2024 19:43
@bpinsard bpinsard force-pushed the enh/complex_realimag branch from eef6f46 to 44544e4 Compare September 24, 2024 17:29
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.

3 participants