search

InsightSoftwareConsortium / ITK

Insight Toolkit (ITK) -- Official Repository. ITK builds on a proven, spatially-oriented architecture for processing, segmentation, and registration of scientific images in two, three, or more dimensions.
https://itk.org
Apache License 2.0
1.41k stars 663 forks source link

Extend GiftiMeshIO to read data with intent NIFTI_INTENT_NONE into PointData #2103

Closed crossmanith closed 3 years ago

crossmanith commented 3 years ago

Description

GiftiMeshIO doesn't read data from a DataArray with Intent="NIFTI_INTENT_NONE" (data written by cat12 Matlab-Toolbox)

Expected behavior

Gifti format documentation states that all data from DataArrays with Intent starting with "NIFTI_INTENT" is functional data. I suggest to read this data into PointData of the mesh.

Actual behavior

DataArray with intent "NIFTI_INTENT_NONE" is skipped

Additional Information

-

thewtex commented 3 years ago

@crossmanith seems reasonable. Can you create a PR with a test case?

crossmanith commented 3 years ago

I'm ready to push but I'm failing with: remote: Permission to InsightSoftwareConsortium/ITK.git denied to crossmanith. fatal: unable to access 'https://github.com/InsightSoftwareConsortium/ITK.git/': The requested URL returned error: 403

To my knowledge I don't need to fork the repository on github. But I guess I configured something wrong during SetupForDevelopment.sh

git status: On branch ITK_2103 .git/config: [remote "origin"] url = https://github.com/InsightSoftwareConsortium/ITK.git fetch = +refs/heads/:refs/remotes/origin/

Hints welcome, Christina

dzenanz commented 3 years ago

You should fork the repository. Only maintainers have write access to InsightSoftwareConsortium/ITK.

crossmanith commented 3 years ago

Now I've pushed to the forked repository. A test case comprises exactly what? A small gifti file? Or some code showing the point data with intent NONE can be read now?

The PR comprises 4 smaller commits with commit messages prefixed with BUG: or STYLE: respectively. For the PR I create a summary message prefixed with BUG: ? Is it optional to add "@mentions"? If not, what name should I put there?

dzenanz commented 3 years ago

A small gifti file? Or some code showing the point data with intent NONE can be read now?

Ideally both. Either is better than none.

PR does not need a prefix, only commits need prefixes. Please do not use @mentions.

crossmanith commented 3 years ago

Now I have code and a gifti file. How do I attach them to the PR?

Additionally I found out that gifticlib returns NIFTI_INTENT_NONE always if the string given by Intent="xxx" is invalid, even if gifticlib states that NIFTI_INTENT_NONE is valid in the comments. This leads to point data for every nonsense intent string. But gifticlib is a third party module. Would this be fixed in ITK?

dzenanz commented 3 years ago

Take a look at how to contribute test data.

As GIFTI is an open-source project, it is probably better to contribute fixes upstream. Maybe just updating to the latest gifticlib might help? The last update was 2 years ago.

crossmanith commented 3 years ago

I've created a PR at NIFTI-Imaging for the gifti_clib related changes.