Display vertebral levels when provided with `-perslice 1` #4004

sandrinebedard · 2023-01-19T22:28:28Z

Checklist

GitHub

I've given this PR a concise, self-descriptive, and meaningful title
I've linked relevant issues in the PR body
I've applied the relevant labels to this PR
I've applied a release milestone (major, minor, patch) in line with Semantic Versioning guidelines
I've assigned a reviewer

PR contents

I've consulted SCT's internal developer documentation to ensure my contribution is in line with any relevant design decisions
I've added relevant tests for my contribution
I've updated the relevant documentation for my changes, including argparse descriptions, docstrings, and ReadTheDocs tutorial pages

Description

This PR adresses #3113. In sct_process_segmentation when specifying -vertfile and -perslice 1, it now specifies the vertebral levels (when they are provided).

Linked issues

Fixes #3113 (and also its duplication #3976)

joshuacwnewton

Thank you for taking on this task, and opening up a PR so quickly! ♥️

I found this PR a little difficult to review, though. So, I have some PR ettiquette-related requests for the future, if possible?

Could you add extended descriptions to your commits, when applicable?

They're really helpful for explaining the reasons for each change. Without explanations, a reviewer either has to has to guess the motivations for each commit, or they have to take the time ask for explanations as part of their review. This slows down the review process, and makes a PR harder to review.

(Obviously sometimes the changes are pretty self-explanatory? But in this case, the motivations for each commit weren't clear to me.)
Could you break down commits into smaller changes?

(I had some trouble understanding 0c21b16, since it has multiple changes that are seemingly unrelated.)

Thank you again!

spinalcordtoolbox/scripts/sct_process_segmentation.py

spinalcordtoolbox/aggregate_slicewise.py

Without this, currently the default value of `argumnets.vert` is `None`. By adding this line, it makes it so that the default value of `arguments.vert` will be an empty list (`[]`).

This allows users to specify `-vert` by itself, assuming they've run `sct_warp_template` and generated the level file in the working directory.

Instead of manually setting `vert` to `[]` and `vertfile` to `None`, we can just rely on their default values.

joshuacwnewton · 2023-01-23T18:46:57Z

I've made some changes to implement the suggestions from the earlier review.

To test these changes, I followed the instructions from the Registration to Template tutorial to register the sct_example_data/t2/t2.nii.gz image to the PAM50 template. Then, I tried computing -perslice metrics on the segmentation, with and without the default ./label/template/PAM50 file specified by the default -vertfile value.

Before running sct_warp_template (vertfile is missing, warning is thrown, no vertlevel info in csv)

(sb/3113-display-vert-level) joshua@tadpole:~/repos/spinalcordtoolbox/data/sct_example_data/t2$ sct_process_segmentation -i t2_seg.nii.gz -z 30:35 -perslice 1 -o csa_perslice.csv

--
Spinal Cord Toolbox (git-sb/3113-display-vert-level-9dd71dc98a637e5aa2175e8149631e43e40265da)

sct_process_segmentation -i t2_seg.nii.gz -z 30:35 -perslice 1 -o csa_perslice.csv
--

Vertebral level file ./label/template/PAM50_levels.nii.gz does not exist. Vert level information will not be displayed. To use vertebral level information, you may need to run `sct_warp_template` to generate the appropriate level file in your working directory.
Compute shape analysis: 100%|##############| 240/240 [00:01<00:00, 194.14iter/s]

Done! To view results, type:
xdg-open /home/joshua/repos/spinalcordtoolbox/data/sct_example_data/t2/csa_perslice.csv

After running sct_warp_template (vertfile is present, no warning, vertlevel info is in csv)

(sb/3113-display-vert-level) joshua@tadpole:~/repos/spinalcordtoolbox/data/sct_example_data/t2$ sct_process_segmentation -i t2_seg.nii.gz -z 30:35 -perslice 1 -o csa_perslice.csv

--
Spinal Cord Toolbox (git-sb/3113-display-vert-level-9dd71dc98a637e5aa2175e8149631e43e40265da)

sct_process_segmentation -i t2_seg.nii.gz -z 30:35 -perslice 1 -o csa_perslice.csv
--

Compute shape analysis: 100%|##############| 240/240 [00:01<00:00, 125.36iter/s]

Done! To view results, type:
xdg-open /home/joshua/repos/spinalcordtoolbox/data/sct_example_data/t2/csa_perslice.csv

Without this check, `aggregate_per_slice_or_level` will try to generate vertebral level information on a nonexistent file.

sandrinebedard · 2023-01-24T15:51:09Z

@joshuacwnewton I've tested it out and works fine on my side!

joshuacwnewton

LGTM! I think we can merge. :)

spinalcordtoolbox/scripts/sct_process_segmentation.py

To be compatible with spinalcordtoolbox/spinalcordtoolbox#4004

sandrinebedard added 3 commits January 19, 2023 16:44

change if condition for arg assignement

3eaf44e

add vertgroup for perslice

0c21b16

remove deflault value of -vertfile

a713bc7

sandrinebedard added the sct_process_segmentation context: label Jan 19, 2023

sandrinebedard requested a review from joshuacwnewton January 19, 2023 22:28

sandrinebedard self-assigned this Jan 19, 2023

joshuacwnewton mentioned this pull request Jan 20, 2023

Refactor the aggregate_per_slice_or_level function to make it easier to understand and modify #4005

Open

joshuacwnewton reviewed Jan 20, 2023

View reviewed changes

remove blank line

78811f4

sandrinebedard requested a review from joshuacwnewton January 20, 2023 20:51

joshuacwnewton added 3 commits January 23, 2023 11:41

sct_process_segmentation: Add default='' to -vert

3e7d96c

Without this, currently the default value of `argumnets.vert` is `None`. By adding this line, it makes it so that the default value of `arguments.vert` will be an empty list (`[]`).

sct_process_segmentation: Restore the default of -vertfile

7bd3fde

This allows users to specify `-vert` by itself, assuming they've run `sct_warp_template` and generated the level file in the working directory.

sct_process_segmentation: Remove the vertfile conditional

8c79374

Instead of manually setting `vert` to `[]` and `vertfile` to `None`, we can just rely on their default values.

sct_process_segmentation: Add existence check for -vertfile

02d25bb

Without this check, `aggregate_per_slice_or_level` will try to generate vertebral level information on a nonexistent file.

joshuacwnewton force-pushed the sb/3113-display-vert-level branch from 9dd71dc to 02d25bb Compare January 23, 2023 18:50

joshuacwnewton approved these changes Jan 24, 2023

View reviewed changes

spinalcordtoolbox/scripts/sct_process_segmentation.py Show resolved Hide resolved

Merge branch 'master' into sb/3113-display-vert-level

babb1cc

sandrinebedard merged commit 2d09af3 into master Jan 24, 2023

joshuacwnewton mentioned this pull request Jan 27, 2023

Default value of -f is confusing (it depends on label/, which must be created by sct_warp_template) #3114

Open

joshuacwnewton deleted the sb/3113-display-vert-level branch February 6, 2023 18:39

valosekj added a commit to sct-pipeline/dcm-metric-normalization that referenced this pull request Feb 15, 2023

Remove '-vert 1:20'

a80c4fe

To be compatible with spinalcordtoolbox/spinalcordtoolbox#4004

joshuacwnewton added this to the 6.0 milestone Jul 13, 2023

joshuacwnewton added the feature category: new functionality label Jul 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display vertebral levels when provided with `-perslice 1` #4004

Display vertebral levels when provided with `-perslice 1` #4004

sandrinebedard commented Jan 19, 2023 •

edited by joshuacwnewton

joshuacwnewton left a comment •

edited

joshuacwnewton commented Jan 23, 2023 •

edited

sandrinebedard commented Jan 24, 2023

joshuacwnewton left a comment

Display vertebral levels when provided with -perslice 1 #4004

Display vertebral levels when provided with -perslice 1 #4004

Conversation

sandrinebedard commented Jan 19, 2023 • edited by joshuacwnewton

Checklist

GitHub

PR contents

Description

Linked issues

joshuacwnewton left a comment • edited

Choose a reason for hiding this comment

joshuacwnewton commented Jan 23, 2023 • edited

sandrinebedard commented Jan 24, 2023

joshuacwnewton left a comment

Choose a reason for hiding this comment

Display vertebral levels when provided with `-perslice 1` #4004

Display vertebral levels when provided with `-perslice 1` #4004

sandrinebedard commented Jan 19, 2023 •

edited by joshuacwnewton

joshuacwnewton left a comment •

edited

joshuacwnewton commented Jan 23, 2023 •

edited