A small functionality added to annotation_utils.py together with a new test for it. #596
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alessandrofelder
requested changes
Jul 9, 2025
| return name, acronym | ||
|
|
||
|
|
||
| def read_itk_labels(path: Path) -> dict: | ||
| def read_itk_labels(path: Path, acryonym_letter=1) -> dict: |
There was a problem hiding this comment.
rename argument as suggested above.
| @@ -3,7 +3,7 @@ | |||
| from pathlib import Path | |||
|
|
|||
|
|
|||
| def split_label_text(name: str) -> str: | |||
| def split_label_text(name: str, acryonym_letter=1) -> str: | |||
| """Split label text into name + acronym. | |||
|
|
|||
| If the label text ends with ')', extract the acronym inside parentheses. | |||
There was a problem hiding this comment.
This docstring needs updating to explain the new function argument:
The "Otherwise, use the first letter as the acronym" part is now wrong!
Co-authored-by: Alessandro Felder <[email protected]>
Co-authored-by: Alessandro Felder <[email protected]>
for more information, see https://pre-commit.ci
Skxsmy
changed the title
alessandrofelder
requested changes
Jul 14, 2025
| @@ -12,27 +13,67 @@ | |||
|
|
|||
|
|
|||
| @pytest.mark.parametrize( | |||
There was a problem hiding this comment.
I would suggest to split this test into three smaller tests, for clarity.
- One test for default (
acronym_length=1) - One test for non-default (
acronym_length=3or similar) - One for error case (
acronym_length = len(atlas_name)+1)
instead of using pytest.param?
| "BGA-N", | ||
| np.random.randint(1), |
There was a problem hiding this comment.
I am confused why you are using randint here?
I would keep it simple and use 1, 2, len+1 for the three tests.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before submitting a pull request (PR), please read the contributing guide.
Please fill out as much of this template as you can, but if you have any problems or questions, just leave a comment and we will help out :)
Description
What is this PR
Why is this PR needed?
Currently the read_itk_labels function only supports 1-letter acryonym
What does this PR do?
Now it allows the user to choose the number of first letters the user want to use as structure acryonym.
How has this PR been tested?
Locally
Is this a breaking change?
no
Does this PR require an update to the documentation?
no
Checklist: