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

Don't convert CIFTIs to int16 #979

Merged
merged 13 commits into from
Oct 30, 2023
Merged

Don't convert CIFTIs to int16 #979

merged 13 commits into from
Oct 30, 2023

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Oct 24, 2023

Closes #977. The problem is that the CIFTI atlas files' contents are being converted to floats.

Changes proposed in this pull request

  • Stop using DerivativesDataSink to write out atlas files. There is a bug in the base DerivativesDataSink where dlabel.nii CIFTIs with non-int16 data fail to convert to int16. Before, I was using nibabel to convert the datatype before feeding the file into the DerivativesDataSink. However, there appears to be a new bug in nibabel where that doesn't work, and changing the datatype in the header actually changes the data array's data type from normal ints to weird floats (e.g., 1 --> 1.46867437), which causes problems when we want to actually use those files.
    • Now, I have a function to compile the right filename and copy atlas file to the output folder.
  • Also, replace the dseg.tsv file for the Gordon atlas, so the labels are more informative (i.e., match the names from Gordon333_FreesurferSubcortical.32k_fs_LR.dlabel.nii from https://sites.wustl.edu/petersenschlaggarlab/parcels-19cwpgu/). This also adds the community labels.
    • I couldn't find communities for Tian or HCP. The 4S atlases have community info for the Schaefer nodes.
  • Replace the dseg.tsv file for the Glass atlas to add communities as well. Thanks to Jakob Seidlitz for the communities.

@tsalo
Copy link
Member Author

tsalo commented Oct 24, 2023

I need to also add community affiliations to the Glasser dseg file, but there's an issue with the label files, so I'll circle back tomorrow.

@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Files Coverage Δ
xcp_d/interfaces/connectivity.py 93.75% <100.00%> (+0.62%) ⬆️
xcp_d/utils/modified_data.py 93.75% <ø> (-0.99%) ⬇️
xcp_d/workflows/connectivity.py 96.93% <100.00%> (-0.07%) ⬇️
xcp_d/utils/atlas.py 98.14% <94.73%> (-1.86%) ⬇️

📢 Thoughts on this report? Let us know!.

@tsalo tsalo added the bug Issues noting problems and PRs fixing those problems. label Oct 27, 2023
@tsalo
Copy link
Member Author

tsalo commented Oct 29, 2023

I just need to write a test for the new copy_atlas function.

@tsalo tsalo merged commit 42f7da6 into PennLINC:main Oct 30, 2023
4 checks passed
@tsalo tsalo deleted the fix-ciftis branch October 30, 2023 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues noting problems and PRs fixing those problems.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Correlation matrices in CIFTI test results are empty
1 participant