-
-
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
Keyword=CombinedLabels
incompatible with combine=1
#4467
Comments
jcohenadad
added
bug
category: fixes an error in the code
sct_extract_metric
context:
labels
May 3, 2024
jcohenadad
changed the title
CombinedLabel incompatible with
May 3, 2024
combine=1
Keyword=CombinedLabels
incompatible with combine=1
jcohenadad
added a commit
that referenced
this issue
May 3, 2024
8 tasks
jcohenadad
added a commit
that referenced
this issue
May 5, 2024
Fixes #4467 <!-- Hi, and thank you for submitting a Pull Request! The checklist below is a brief summary of steps found in the NeuroPoly Contributing Guidelines, which can be found here: https://intranet.neuro.polymtl.ca/onboarding/software-development.html#software-development. --> ## Checklist #### GitHub - [x] I've given this PR a concise, self-descriptive, and meaningful title - [x] I've linked relevant issues in the PR body - [x] I've applied [the relevant labels](https://intranet.neuro.polymtl.ca/onboarding/software-development.html#pr-labels) to this PR - [x] I've applied a [release milestone](https://github.com/spinalcordtoolbox/spinalcordtoolbox/milestones) (major, minor, patch) in line with [Semantic Versioning guidelines](https://github.com/spinalcordtoolbox/spinalcordtoolbox/wiki/Misc%3A-Creating-a-new-release#convention-for-naming-releases) - [ ] I've assigned a reviewer <!-- For the title, please observe the following rules: - Provide a concise and self-descriptive title - Do not include the applicable issue number in the title, do it in the PR body - If the PR is not ready for review, convert it to a draft. --> #### PR contents - [ ] I've consulted [SCT's internal developer documentation](https://github.com/spinalcordtoolbox/spinalcordtoolbox/wiki) to ensure my contribution is in line with any relevant design decisions - [ ] I've added [relevant tests](https://github.com/spinalcordtoolbox/spinalcordtoolbox/wiki/Testing) for my contribution nope 😢 (see #4468) - [ ] I've updated the [relevant documentation](https://github.com/spinalcordtoolbox/spinalcordtoolbox/wiki/Documentation) for my changes, including argparse descriptions, docstrings, and ReadTheDocs tutorial pages ## Description <!-- describe what the PR is about. Explain the approach and possible drawbacks.It's ok to repeat some text from the related issue. --> I'm having issues creating a test for it (#4468), we should add the following tests for `test_cli_sct_extract_metric.py`: - Labels under `Keyword=IndivLabels`: `-l 2,3 -combine 1` --> it should combine 2,3 into a single label - Labels under `Keyword=IndivLabels`: `-l 2 -combine 1` --> it should not crash, and extract in label 2 - Labels under `Keyword=CombinedLabels`: `-l 52 -combine 1` --> it should not crash, and extract in label 52 (the test I was trying to make) - Labels under `Keyword=CombinedLabels`: `-l 51,52 -combine 1` --> Now that's an interesting one. I'm now sure what will happen in the code to be honest 😅 but ideally we would produce a label (99) that combines all tracts within 51 and 52 (and deal with the redundancy) ## Linked issues <!-- If the PR fixes any issues, indicate it here with issue-closing keywords: e.g. Resolves #XX, Fixes #XX, Addresses #XX. Note that if you want multiple issues to be autoclosed on PR merge, you must use the issue-closing verb before each relevant issue: e.g. Resolves #1, Resolves #2 --> --------- Co-authored-by: Joshua Newton <joshuacwnewton@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Context
When running
sct_extract_metric
with a combination the flag-l
that points to a labelKeyword=CombinedLabels
, i.e., with values 50 to 55, while using the flag-combine 1
, the script crashes:While when setting
combine 0
, the script runs:Although it does not really make sense to combine tracts that are already combined, this crash is not intuitive for the user.
Solution
Deal with the condition 'CombinedLabel' &
combine=1
.The text was updated successfully, but these errors were encountered: