perklab / slicerfreesurfer Goto Github PK

Exporting a segmentation does not currently guarantee that labelmap values will match the expected values.

FreeSurfer segmentations are currently imported and assigned to segments using the FreeSurfer values, however the label values may not be the same on export (see related issue here).

Segmentation are currently only saved as NRRD, however we should add the option to export the Segmentations to other FreeSurfer formats (NIFTI, MGZ, etc.)

Related discussion https://discourse.slicer.org/t/export-segmentation-scene-mrml-and-segmentations-nrrd-into-nifti-format-for-dropbox-viewing/12517.

The current user interface is not very efficient.

It would be nice to show a tree-style data view similarly to FreeView: https://youtu.be/Mv-ECowxq2o?t=258 to reduce the amount of clicks and scrolling.

There is a minor build error on linux and mac:
http://slicer.cdash.org/viewBuildError.php?buildid=1898890

.toUtf8() or .toUtf8().constData() should work

After the issue is fixed (and maybe after #1 is fixed, too) and the extension shows up correctly on the extension manager, it would be nice if could announce the feature on youtube+discourse. Thank you!

Here is the initial mri from the free dataset: https://openneuro.org/datasets/ds004776/versions/1.0.0

https://s3.amazonaws.com/openneuro.org/ds004776/sub-01/anat/sub-01_T1w.nii.gz?versionId=lob8sYgfY.jRec5aLib_K73v_.NBAeQB

Here is the mrprage.nii.gz file from the pipeline (input grayscale volume):
mprage.nii.gz.zip

Here is the lh.pial (for example) after running infant_recon_all:
lh.pial.zip

Direction Matrix:

Opening mprage.nii.gz as a regular volume and opening lh.pial as a Freesurfer model with mprage.nii.gz as reference:

However, freeview reads and align the two files correctly.

/tmp/SlicerFreeSurfer/FreeSurfer/vtkFSSurfaceAnnotationReader.cxx: In member function ‘int vtkFSSurfaceAnnotationReader::ReadFSAnnotation()’:
/tmp/SlicerFreeSurfer/FreeSurfer/vtkFSSurfaceAnnotationReader.cxx:353:26: warning: passing argument 1 to restrict-qualified parameter aliases with argument 3 [-Wrestrict]
  353 |           sprintf (this->NamesList, "%s%3d {%s} ", this->NamesList,
      |                    ~~~~~~^~~~~~~~~  

If copying takes place between objects that overlap as a result of a call to sprintf() or snprintf(), the results are undefined.

Source: https://stackoverflow.com/a/1973595/1539918

And answer from ChatGPT:

This warning suggests that there is a potential issue with the use of the restrict keyword in the function argument, and it is causing an aliasing problem.

The restrict keyword is a C99 feature that tells the compiler that a particular pointer argument is not aliased by any other pointer in the same scope. In other words, if you use the restrict keyword, you're telling the compiler that the pointer passed as an argument does not point to the same memory location as any other pointer passed to the same function.

In your case, passing this->NamesList as the first argument to sprintf with %s format specifier is causing the warning. The %s format specifier expects a null-terminated string, and since the sprintf function can write to the memory location pointed to by this->NamesList, it is possible that it can also write to memory locations pointed to by other pointers. This violates the restrict keyword, which is meant to prevent such aliasing.

To fix this warning, you should consider using a different approach to write to this->NamesList. You can either remove the restrict keyword from the function argument, or you can use a temporary buffer to store the formatted string and then copy it to this->NamesList using strcpy or strncpy. Here is an example of the second approach:

char tempBuffer[256]; // temporary buffer to store the formatted string
sprintf(tempBuffer, "%s%3d {%s} ", this->NamesList, someNumber, someString);
strncpy(this->NamesList, tempBuffer, sizeof(this->NamesList));
this->NamesList[sizeof(this->NamesList)-1] = '\0'; // make sure the string is null-terminated

In this example, we use sprintf to format the string into tempBuffer, and then we use strncpy to copy the formatted string to this->NamesList. We also make sure that the string is null-terminated by setting the last character to '\0'. This approach avoids the aliasing issue and should fix the warning.

No volume information contained in the file
Traceback (most recent call last):
  File "/path/Software/Slicer/Slicer-5.6.1-linux-amd64/slicer.org/Extensions-32438/SlicerFreeSurfer/lib/Slicer-5.6/qt-scripted-modules/NiBabelModelIO.py", line 169, in readModelNode
    geom = VolGeom.from_surf_footer(metadata)
  File "/path/Software/Slicer/Slicer-5.6.1-linux-amd64/slicer.org/Extensions-32438/SlicerFreeSurfer/lib/Slicer-5.6/qt-scripted-modules/NiBabelModelIO.py", line 70, in from_surf_footer
    shape=footer['volume'],
KeyError: 'volume'
[Qt] "/path/Software/Slicer/Slicer-5.6.1-linux-amd64/slicer.org/Extensions-32438/SlicerFreeSurfer/lib/Slicer-5.6/qt-scripted-modules/NiBabelModelIO.py"  - In "NiBabelModelIOFileReader" class, function 'write'  is expected to return a string boolean !
[Qt] static void qSlicerIOManager::showLoadNodesResultDialog(bool, vtkMRMLMessageCollection*) Errors occurred while loading nodes: "Error: Loading /tmp/lh-filled-outer-surface.pial - Could not load file: /tmp/lh-filled-outer-surface.pial\n"

lh-filled-outer-surface.zip

The file was saved using MRISwrite function in a custom mris. Freeview is able to load the file.
This must be related to changes introduced to solve #17

Currently, the extension shows up in "Examples" category. It should be something like "Neuro", "Neuroimaging", "Brain imaging",... Maybe ask Sylvain and Jared if they have any preferred name. It should be a name that will be suitable to host the new modules that we'll develop for them.

Slicer/ExtensionsIndex#1693 (comment)

Hello, I recently spent some time figuring out how Slicer transformation matrices are related to the FSL tool FLIRT's transformation matrices (info and discussion here: https://discourse.slicer.org/t/importing-fsl-flirt-transformation-matrices/30819/8?u=mikebind), and @pieper suggested that it might make sense to include this capability in SlicerFreeSurfer. If you agree and can give me some guidance, I'm happy to try to put together a PR with some python code to do the loading. Alternatively, if someone here wants to adapt the code from the gist (e.g. if that would be faster and easier than the time it would take to give me guidance on how to best fit it in with the existing modules), please feel free to use or adapt anything useful from my rough code here https://gist.github.com/mikebind/d116527e29458637602c86b1d4cd8c2c (no licensing restrictions whatsoever). Also, I fully recognize that FSL and FreeSurfer are different tools and it might not make sense to include this functionality at all in SlicerFreeSurfer, I was just following up on the suggestion.

Should we merge SlicerFreeSurfer and SlicerFreeSurferCommands, and move them to the Slicer GitHub organization?

Please see the discussion here for more details.

One thing to consider is that the SlicerFreeSurfer extension doesn't have any external dependencies as far as I'm aware, whereas the SlicerFreeSurferCommands extension depends on FreeSurfer which needs to be installed and configured by the user, takes up about 14 GB of disk space, and can only be installed on Linux/Unix OS (on Windows WSL can be used).

Some freesurfer tools (e.g. robust register) output RAS to RAS transforms in .lta format, which is not supported by this extension.

This snippet of Slicer code loads the file and the result appears to be correct, but I don't know how much variability there is in the format or if it's even documented. I don't know if the other lines in the file have any particular meaning.

ltaPath = "/data/affine.lta"
lta = open(ltaPath).read().split("\n")
mat = vtk.vtkMatrix4x4()
for row in range(4):
    print(row, lta[8+row])
    elems = lta[8+row].split(" ")
    for col in range(4):
        mat.SetElement(row, col, float(elems[col]))
transform = slicer.mrmlScene.AddNewNodeByClass("vtkMRMLLinearTransformNode")
transform.SetMatrixTransformToParent(mat)

Here's an example .lta file:

# transform file /mnt/c/Users/mbrudfors/Projects/neurosurgical-atlas/Data/Ture-028-2022-10-07-Scene/Data/affine.lta
# created by mbrudfors on Thu Oct 27 11:07:23 2022

type      = 1 # LINEAR_RAS_TO_RAS
nxforms   = 1
mean      = 80.0000 107.0000 116.0000
sigma     = 10000.0000
1 4 4
1.004464387893677e+00 6.770147383213043e-02 -1.217468529939651e-01 -1.079803466796875e+00 
-9.191786497831345e-02 9.181172251701355e-01 -1.942616552114487e-01 3.466661071777344e+01 
1.277457177639008e-01 1.542867422103882e-01 1.004784703254700e+00 3.302518463134766e+01 
0.000000000000000e+00 0.000000000000000e+00 0.000000000000000e+00 1.000000000000000e+00 
src volume info
valid = 1  # volume info valid
filename = /mnt/c/Users/mbrudfors/Projects/neurosurgical-atlas/Data/Ture-028-2022-10-07-Scene/Data/ss_Moving_Volume.nii.gz
volume = 160 220 220
voxelsize = 1.000000000000000e+00 1.000000000000000e+00 1.000000000000000e+00
xras   = 1.000000000000000e+00 0.000000000000000e+00 0.000000000000000e+00
yras   = 0.000000000000000e+00 1.000000000000000e+00 0.000000000000000e+00
zras   = -0.000000000000000e+00 -0.000000000000000e+00 1.000000000000000e+00
cras   = 1.167999267578125e+00 -1.290399932861328e+01 1.227996826171875e+00
dst volume info
valid = 1  # volume info valid
filename = /mnt/c/Users/mbrudfors/Projects/neurosurgical-atlas/Data/Ture-028-2022-10-07-Scene/Data/ss_Fixed_Volume.nii.gz
volume = 512 512 392
voxelsize = 5.077999830245972e-01 5.077999830245972e-01 5.000000000000000e-01
xras   = -1.000000000000000e+00 0.000000000000000e+00 0.000000000000000e+00
yras   = 0.000000000000000e+00 -1.000000000000000e+00 0.000000000000000e+00
zras   = 0.000000000000000e+00 0.000000000000000e+00 1.000000000000000e+00
cras   = -2.550689697265625e+00 2.907742309570312e+01 2.825936889648438e+01

We can currently import FreeSurfer files using the FreeSurferImporter module, however we cannot export them into a FreeSurfer-compatible format.

When I try to load a freesurfer segmentation (a standard aparc+aseg.mgz) on Ubuntu (either using the importer interface, the load volume interface or a slicerpython command) I will get this error:

“Slicer has caught an application error, please save your work and restart.\n\nThe application has run out of memory. Increasing swap size in system settings or adding more RAM may fix this issue.\n\nIf you have a repeatable sequence of steps that causes this message, please report the issue following instructions available at https://slicer.org\n\n\nThe message detail is:\n\nException thrown in event: std::bad_alloc”

I’m using last version of Slicer and FreeSurferImporter on Ubuntu 23.04. I have 8gb RAM memory and I can load without error this segmentation in the same machine windows partition. Moreover a collegue had the same error on a linux high performance machine.
I also tried with standard freesurfer course data (ftp://surfer.nmr.mgh.harvard.edu/pub/data/tutorial_data/buckner_data/tutorial_subjs/good_output/mri/brain.mgz and ftp://surfer.nmr.mgh.harvard.edu/pub/data/tutorial_data/buckner_data/tutorial_subjs/good_output/mri/aparc+aseg.mgz), and I’m still getting the same error on linux.

https://discourse.slicer.org/t/error-loading-freesurfer-segmentation/32674/2

Cxx17 is now used in Slicer, which caused a build error:

https://slicer.cdash.org/build/2733879

Official freesurfer sample data set is 8GB. This is quite extreme. It is also quite odd that they describe 3 linux commands to download a package instead of simply posting a download link (strange people...).

It would be much better for users if we made available one or few sample data sets in SampleData module.

You can prepare a nice scene, save as mrb, and upload it as Release asset in the repository (create a "Data" release and upload all sample and test data there). You can write SampleData plugin similarly to this: https://github.com/SlicerRt/Sequences/tree/master/SequenceSampleData

> Reduce noise

@Sunderlandkyl

To remove the "noise" associated with the project, I suggest disabling the following unused features if it still applies:

Go to General settings and disable the Wiki, Projects and Discussions tabs	If empty or unused, hide Releases and Packages section on the right side

Rational

It is just to simplify things.

For example, if user support is provided via a forum then it is better to turn off Discussions here, so that there is no need to monitor two user support sites, but only one.

If Releases and Packages are not used, so it is cleaner to hide them (to indicate that it is not the way how we manage releases and packages).

Projects feature is rarely used in general and this repository does not use it either. If the user clicks on Projects then an empty page is s displayed. It is better to not display the heading at all, to save the extra two clicks for users exploring the project.

Wiki on Github is meant to be a lighter weight tool for documentation. The issue is that it is too limited, for example contributors either can make changes directly (without pull request) or they cannot make changes at all. It also lead to write redundant content, since Github quite nicely renders markdown pages in the main repository. For small projects it is simpler and more consistent to keep all documentation in markdown files in the main repository.

Co-authored by @lassoan

_{This comment was adapted from my GitHub saved replies, it is licensed under a Creative Commons Attribution 4.0 International License and you were welcome to reuse it.}