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

Add create surface from labels #1005

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

Conversation

arnaudbore
Copy link
Contributor

@arnaudbore arnaudbore commented Jun 22, 2024

Quick description

First attempt to bring some of the nice scripts from Etienne to scilpy.
I choose to reduce the scope of the original script to fit scilpy requirements
I kept the expected behavior of the script used in nf-set.

...

Type of change

Check the relevant options.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Provide data, screenshots, command line to test (if relevant)

Use scilpy atlas test data
...

Checklist

  • My code follows the style guidelines of this project (run autopep8)
  • I added relevant citations to scripts, modules and functions docstrings and descriptions
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I moved all functions from the script file (except the argparser and main) to scilpy modules
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@pep8speaks
Copy link

Hello @arnaudbore, Thank you for updating !

Line 137:80: E501 line too long (86 > 79 characters)

Copy link
Member

@frheault frheault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this script should work with a single mask (uint8) as well as labels (uint16), Like if I want a glass brain right now I have to artificially convert my BET mask to unint16

p.add_argument('--fill', action='store_true',
help='Fill holes in the image.')

p.add_argument('--vox2vtk', action='store_true',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I say the default should open in MI-Brain correctly on top of the NIFTI it was generated for. Should switch to --vtk2vox

labels_volume = get_data_as_labels(labels_img)

# Removed indices
mask = get_binary_mask_from_labels(labels_volume, args.indices)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As of now, this is crashing unless given (so I would use required at least).
But since this is a script <from_labels> I think the default should be:
generate all the labels (from np.unique() then the script does the for loop itself) into an output folder, if the user wants just a few then --indices uses just one.
similar to the split labels script we could use an --out_dir OR --out_suffix .

This is my recommendation because if you run this on Freesurfer atlas, most people don't know the labels one by one and they probably want to generate a bunch of independent surface. And if they want to merge a few labels into a single mask, I propose that @arnaudbore you reuse the same logic as in scil_tractogram_filter_with_roi.py (like --indices 1 2 3 4 (do only 4 surfaces instead of all) VS --merge_indices 1:6 9 10:15 (do a single surface from a bunch of labels) and these two would be mutually exclusive options (default being all labels as multiple independent surfaces)

Example : use wmparc.a2009s.nii.gz with some aseg.stats indices

scil_surface_from_volume.py s1a1/mask/S1-A1_wmparc.a2009s.nii.gz\\
-v -index 16 --vox2vtk -opening 2 -smooth 2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

index?

--opening --smooth

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

3 participants