Output a better error message when the initial disc is invalid #3775

mguaypaq · 2022-04-20T21:46:31Z

Description

When calling vertebral_detection with an invalid initial disc (that is, a disc label that's not in the PAM50 template), the result is a very unfriendly error message:

Traceback (most recent call last):
  File "/icometrix-install/spinalcordtoolbox-5.3.0/spinalcordtoolbox/vertebrae/core.py", line 206, in vertebral_detection
    approx_distance_to_next_disc = list_distance[list_disc_value_template.index(current_disc - 1)]
ValueError: -2 is not in list
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
  File "/icometrix-install/spinalcordtoolbox-5.3.0/spinalcordtoolbox/scripts/sct_label_vertebrae.py", line 482, in <module>
    main(sys.argv[1:])
  File "/icometrix-install/spinalcordtoolbox-5.3.0/spinalcordtoolbox/scripts/sct_label_vertebrae.py", line 419, in main
    verbose=verbose, path_template=path_template, path_output=path_output, scale_dist=scale_dist)
  File "/icometrix-install/spinalcordtoolbox-5.3.0/spinalcordtoolbox/vertebrae/core.py", line 208, in vertebral_detection
    logger.warning('Disc value not included in template. Using previously-calculated distance: %s', approx_distance_to_next_disc)
UnboundLocalError: local variable 'approx_distance_to_next_disc' referenced before assignment

(This can only happen in the first iteration through the loop in vertebral_detection(). In further iterations, the error is caught, a warning is displayed, and the previous iteration's value of approx_distance_to_next_disc is used.)

This PR adds a check to detect this condition up-front and output a (hopefully) friendlier message to the user.

This is not a complete fix for the linked issue (#3673, see also the original forum post), but is likely the best we can do in time for the 5.6 release.

Linked issues

Part of #3673.

For consistency with the rest of the code, where a 'disc' is a (z, value) pair.

Logically, `current_value` is a disc label, not a list index. `list_disc_z_template` and `list_disc_value_template` are twin lists, so using `list_disc_value_template.index()` is the way to convert a disc label into a relevant list index. This bug was not apparent because, at least for the default template, `list_disc_value_template == [0, 1, 2, ..., 19]`.

We need `current_value` in `list_disc_value_template` to start (otherwise 'Reached the bottom of the template. Stop searching.') We also need `current_value - 1` in there to compute the first `approx_distance_to_next_disc`, since for the first iteration we can't 'Using previously-calculated distance.' (Note that if `current_value` is actually in the template, then `current_value - 1` will automatically be added to `list_disc_value_template`, see `# add disc above top one`.)

mguaypaq · 2022-04-21T00:17:21Z

Looks like test_sct_label_vertebrae_high_value_warning is failing. I'll investigate.

This reverts commit 8811845. The code which looks like a type error is specifically exercised by the test suite, so I'm restoring the code until I investigate further.

This currently fails in a different part of the code, with an ugly error message. This commit adds a check and a better error message.

At least for the (default) PAM50 template, there are 3 cases when using a high initial disc value: value=19: the code does the 'superior' direction of the loop, then does two iterations in the 'inferior' direction, and emits a warning for 'Disc value not included in template.' This is unchanged by this commit. value=20: the code does the 'superior' direction of the loop, then emits an ugly ValueError traceback while switching to the 'inferior' direction because the initial disc was the most 'inferior' disc already. This should arguably not be an error, so the new code instead emits an informational log. value=21 (or more): the initial disc value is outside the template, so the code detects this condition and errors out with a sensible error message. This is unchanged by this commit.

These new test cases are more about illustrating the current behaviour of the code on corner cases, than about what we would want the bahaviour to be. They should be updated and/or removed when we improve the code itself.

mguaypaq · 2022-04-21T04:39:02Z

Ok, this is ready for review again. I'm very open to the suggestion that we shouldn't merge the new test cases into master, but I wanted to include them in the PR mostly as an illustration of various corner cases (and proof that the new errors/warnings do get triggered).

I tried to plug as many holes as possible with decent error messages, but in my mind this further cements the idea that we should take a good look at the vertebral detection code after the 5.6 release.

codecov · 2022-04-21T04:45:40Z

Codecov Report

Merging #3775 (972ceae) into master (a9af30e) will increase coverage by 0.04%.
The diff coverage is 96.87%.

Flag	Coverage Δ
api-tests	`22.49% <0.00%> (-0.03%)`	⬇️
cli-tests	`58.55% <96.87%> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
spinalcordtoolbox/vertebrae/core.py	`75.59% <95.45%> (+1.39%)`	⬆️
spinalcordtoolbox/scripts/sct_label_vertebrae.py	`72.19% <100.00%> (+1.03%)`	⬆️

joshuacwnewton

I'm very open to the suggestion that we shouldn't merge the new test cases into master, but I wanted to include them in the PR mostly as an illustration of various corner cases (and proof that the new errors/warnings do get triggered).

I found these corner case tests to be very valuable for reviewing.

(What I did locally was to rebase the "adding tests" commit to be the first commit, run the tests prior to your changes, then step through your changes to see what the effect on the tests would be before/after, making it act as a sort of TDD-esque changeset.)

test_sct_label_vertebrae_initial_disc_no_inferior: ValueError: 20 is not in list (Fixed by a442e12)
test_sct_label_vertebrae_initial_disc_zero: spinalcordtoolbox.vertebrae.core.EmptyArrayError: Center of mass can't be calculated on empty arrays. (Fixed by 16dbd6f)
test_sct_label_vertebrae_initial_disc_too_low: ValueError: min() arg is an empty sequence (Fixed by b8e146f)
test_sct_label_vertebrae_initial_disc_too_high: ValueError: 21 is not in list (Fixed by b8e146f)

So, thank you for including these tests. :)

I've included one comment, but it's just about documentation for the tests, so I'm going to pre-approve.

testing/cli/test_cli_sct_label_vertebrae.py

Dimissing review because I realized I had a separate comment beyond the 4 added test cases

spinalcordtoolbox/vertebrae/core.py

mguaypaq added 4 commits April 20, 2022 17:03

Fix typo in comment

a35db05

Rename variable current_disc -> current_value

d10244b

For consistency with the rest of the code, where a 'disc' is a (z, value) pair.

mguaypaq added bug category: fixes an error in the code sct_label_vertebrae context: SCT API: vertebrae context: labels Apr 20, 2022

mguaypaq added this to the 5.6 milestone Apr 20, 2022

mguaypaq requested a review from joshuacwnewton April 20, 2022 21:46

mguaypaq self-assigned this Apr 20, 2022

mguaypaq added 5 commits April 20, 2022 21:00

Revert "Fix a type error"

bd4762e

This reverts commit 8811845. The code which looks like a type error is specifically exercised by the test suite, so I'm restoring the code until I investigate further.

Fix typo 'disk' -> 'disc'

510277d

Another corner case: -initz <z-level>,0

16dbd6f

This currently fails in a different part of the code, with an ugly error message. This commit adds a check and a better error message.

Add test cases for the corner cases

3f7832a

These new test cases are more about illustrating the current behaviour of the code on corner cases, than about what we would want the bahaviour to be. They should be updated and/or removed when we improve the code itself.

joshuacwnewton previously approved these changes Apr 22, 2022

View reviewed changes

testing/cli/test_cli_sct_label_vertebrae.py Show resolved Hide resolved

joshuacwnewton reviewed Apr 22, 2022

View reviewed changes

spinalcordtoolbox/vertebrae/core.py Show resolved Hide resolved

mguaypaq added 2 commits April 22, 2022 14:07

Add a description of three test cases

af579b5

Merge branch 'master' into mgp/neg_disc

972ceae

mguaypaq requested a review from joshuacwnewton April 22, 2022 18:08

joshuacwnewton approved these changes Apr 23, 2022

View reviewed changes

mguaypaq merged commit d01260c into master Apr 23, 2022

mguaypaq deleted the mgp/neg_disc branch April 23, 2022 21:15

joshuacwnewton mentioned this pull request Apr 25, 2022

Throw a better error when the center of mass of the label image is outside of the labeled region #3673

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Output a better error message when the initial disc is invalid #3775

Output a better error message when the initial disc is invalid #3775

mguaypaq commented Apr 20, 2022 •

edited by joshuacwnewton

mguaypaq commented Apr 21, 2022

mguaypaq commented Apr 21, 2022

codecov bot commented Apr 21, 2022 •

edited

joshuacwnewton left a comment •

edited

Output a better error message when the initial disc is invalid #3775

Output a better error message when the initial disc is invalid #3775

Conversation

mguaypaq commented Apr 20, 2022 • edited by joshuacwnewton

Description

Linked issues

mguaypaq commented Apr 21, 2022

mguaypaq commented Apr 21, 2022

codecov bot commented Apr 21, 2022 • edited

Codecov Report

joshuacwnewton left a comment • edited

Choose a reason for hiding this comment

mguaypaq commented Apr 20, 2022 •

edited by joshuacwnewton

codecov bot commented Apr 21, 2022 •

edited

joshuacwnewton left a comment •

edited