-
-
Notifications
You must be signed in to change notification settings - Fork 101
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
sct_deepseg_sc/lesion: Allow to input manual or semi-manual centerline #2020
Conversation
scripts/sct_deepseg_lesion.py
Outdated
@@ -51,14 +51,14 @@ def get_parser(): | |||
mandatory=True, | |||
example=['t2', 't2_ax', 't2s']) | |||
parser.add_option(name="-centerline", | |||
type_value="multiple_choice", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you want to keep "multiple_choice", otherwise the values "cnn" and "svm" will be considered as a type:nifti_input
wheres they should be considered as type:string
by the Parser.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
related to #2020 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My answer here: #2020 (comment)
scripts/sct_deepseg_lesion.py
Outdated
type_value="multiple_choice", | ||
description="choice of spinal cord centerline algorithm.", | ||
type_value="image_nifti", | ||
description="Method used for extracting the centerline.\nsvm: automatic centerline detection, based on Support Vector Machine algorithm.\ncnn: automatic centerline detection, based on Convolutional Neural Network.\nviewer: semi-automatic centerline generation, based on manual selection of a few points using an interactive viewer, then approximation with NURBS.\nprovide the filename of a manual centerline (e.g. t2_centerline_manual.nii.gz).\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part of the doc is a bit confusing wrt. input file name (it is not clear what one should write after -centerline
for inputing a file name. I would rewrite as:
type_value="multiple_choice",
description="Method used for extracting the centerline.
"\nsvm: automatic centerline detection, based on Support Vector Machine algorithm."
"\ncnn: automatic centerline detection, based on Convolutional Neural Network."
"\nviewer: semi-automatic centerline generation, based on manual selection of a few points using an interactive viewer, then approximation with NURBS."
"\nFILENAME: filename of an existing centerline (e.g. t2_centerline_manual.nii.gz).\n",
Now, the problem is that my suggestion above will NOT work, because multiple_choice
will look for the string FILENAME
instead of an actual file name :-(
I see two alternatives:
- replace
FILENAME
bymanual
and have another flag dedicated to inputing a centerline file, e.g.:sct_deepseg_sc -i t2.nii.gz -c t2 -centerline manual -filecenterline t2_centerline_manual.nii.gz
. Cons: heavy. - modify the Parser to have the possibility to not check the input string when using
multiple_choice
. Cons: defeat the purpose of a parsing checker...
@zougloub any idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jcohenadad: I initially tried with multiple_choice
and I faced the same issue as you just mention above.
As you said, I used type:image_nifti
to allow the user to input a manul centerline. While the other options (i.e. svm
, cnn
, viewer
) are not considered as Image
, from my understanding, thanks to the parameter list_no_image=['svm', 'cnn', 'viewer']
.
--> This approach is inspired from PropSeg:
https:/neuropoly/spinalcordtoolbox/blob/master/scripts/sct_propseg.py#L242
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of the doc corrected as suggested by @jcohenadad, in commit #f34bf6b.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the problem is that the way this is currently done in PropSeg is not good either (for the same reasons as here). It would be nice to find a proper solution and then deploy it to the rest of SCT. Waiting for @zougloub's feedback there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jcohenadad : @zougloub approved the PR, should I merge the branch or wait for further discussions about the point you raised?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i suggest the following:
"svm: automatic centerline detection, based on Support Vector Machine algorithm."
"cnn: automatic centerline detection, based on Convolutional Neural Network."
"viewer: semi-automatic centerline generation, based on manual selection of a few points using an interactive viewer, then approximation with NURBS."
"manual: filename of an existing centerline (e.g. t2_centerline_manual.nii.gz).\n",
and another flag:
-file-centerline; type:type:image_nifti; description: Input centerline file (to use with flag -centerline manual)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jcohenadad : I have made the modifications we discussed yesterday.
Ready to be reviewed.
scripts/sct_deepseg_sc.py
Outdated
# average centerline coordinates over slices of the image | ||
x_centerline_fit_rescorr, y_centerline_fit_rescorr, z_centerline_rescorr, x_centerline_deriv_rescorr, y_centerline_deriv_rescorr, z_centerline_deriv_rescorr = centerline.average_coordinates_over_slices(image_labels) | ||
|
||
# compute z_centerline in image coordinates for usage in vertebrae mapping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure i see the link with vertebral mapping here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected in commit #86fa0db
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some questions
scripts/sct_deepseg_lesion.py
Outdated
@@ -190,6 +190,9 @@ def deep_segmentation_MSlesion(fname_image, contrast_type, output_folder, ctr_al | |||
tmp_folder = sct.TempFolder() | |||
tmp_folder_path = tmp_folder.get_path() | |||
fname_image_tmp = tmp_folder.copy_from(fname_image) | |||
if os.path.isfile(ctr_algo): # if the ctr_algo is a manual centerline file | |||
ctr_algo = os.path.basename(ctr_algo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why take the base name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the same reason as:
https:/neuropoly/spinalcordtoolbox/blob/master/scripts/sct_deepseg_lesion.py#L189
--> If we pass the if condition, then the ctr_algo
is the filename of a nifti mask.
Do you recommend to change the name of the variable (initially this variable reffered to algorithm name)? to change the if condition (ie check if it is a nifti image)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just want to know why you are trimming the path to its basename, why not keep a full path or absolute path. What if the file is in another folder?
Note: I just took like 1 minute to do a very quick review your code, I didn't look at the implications, just asking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hehe! very good point thank you.
I swap the two lines (see commit ccd068a) : now I copy the file in the tmp folder then I extract the basename --> which will be usefull in the remaining of the code when processing in the tmp_folder.
scripts/sct_deepseg_sc.py
Outdated
@@ -136,7 +136,7 @@ def crop_image_around_centerline(filename_in, filename_ctr, filename_out, crop_s | |||
x_lst, y_lst = [], [] | |||
data_im_new = np.zeros((crop_size, crop_size, im_in.dim[2])) | |||
for zz in range(im_in.dim[2]): | |||
if 1 in np.array(data_ctr[:, :, zz]): | |||
if np.sum(np.array(data_ctr[:, :, zz])): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could also use np.any
for more semantic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
… into cg_deepseg_viewer_centerline
…ropoly/spinalcordtoolbox into cg_deepseg_viewer_centerline merge
scripts/sct_deepseg_lesion.py
Outdated
sys.exit(1) | ||
if "-file_centerline" in args: | ||
manual_centerline_fname = arguments["-file_centerline"] | ||
if ctr_algo != 'manual': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alternatively we can automatically set ctr_algo='manual'
(or ignore that field) if user enters a manual centerline. It will make lighter syntax.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion, thank you. Done in the last commit.
sct_deepseg_sc/lesion: allow to input manual or semi manual centerline Former-commit-id: b33a7b3
Rational:
In some cases, the centerline detection (svm- or cnn-based) fails, and it would be nice to be able to bypass this step by inputing our own centerline (e.g., obtained semi- or fully-manually).
New features:
A new option in the flag "centerline", where a file name or "viewer" can be input.
Example of usage with the viewer:
Example of usage with the manual centerline:
Approach:
algo_fitting='nurbs', nurbs_pts_number=10000
.Related issues: