Rename NormalizeLabelsInDatasetd to RemapLabelsToSequentiald and fix label ordering bug #8680
Conversation
📝 WalkthroughWalkthroughAdds a new MapTransform RemapLabelsToSequentiald that deterministically remaps label values to a sequential range: background forced to 0 and non-background labels assigned 1..N based on alphabetical order of label names; undefined labels become background (0). The transform updates both the label mask channel and d["label_names"]. Introduces a deprecated wrapper NormalizeLabelsInDatasetd (decorated with Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/apps/deepedit/test_deepedit_transforms.py (1)
334-348: Remove unused variable assignment.Line 343 assigns
resultbut never uses it. Since the test focuses on the deprecation warning, the assignment can be removed.🔎 Proposed fix
with warnings.catch_warnings(record=True) as w: warnings.simplefilter("always") transform = NormalizeLabelsInDatasetd(keys="label", label_names={"spleen": 1, "background": 0}) - result = transform(data) + transform(data) # Check that a deprecation warning was raised self.assertEqual(len(w), 1)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
monai/apps/deepedit/__init__.pymonai/apps/deepedit/transforms.pytests/apps/deepedit/test_deepedit_transforms.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
monai/apps/deepedit/__init__.pymonai/apps/deepedit/transforms.pytests/apps/deepedit/test_deepedit_transforms.py
🧬 Code graph analysis (3)
monai/apps/deepedit/__init__.py (1)
monai/apps/deepedit/transforms.py (14)
AddGuidanceFromPointsDeepEditd(677-750)AddGuidanceSignalDeepEditd(229-317)AddInitialSeedPointDeepEditd(363-474)AddInitialSeedPointMissingLabelsd(814-925)AddRandomGuidanceDeepEditd(547-674)DiscardAddGuidanced(36-84)FindAllValidSlicesDeepEditd(320-360)FindAllValidSlicesMissingLabelsd(928-970)FindDiscrepancyRegionsDeepEditd(477-544)NormalizeLabelsInDatasetd(158-177)RemapLabelsToSequentiald(87-155)ResizeGuidanceMultipleLabelDeepEditd(753-792)SingleLabelSelectiond(180-226)SplitPredsLabeld(795-811)
monai/apps/deepedit/transforms.py (1)
monai/transforms/transform.py (1)
MapTransform(372-497)
tests/apps/deepedit/test_deepedit_transforms.py (1)
monai/apps/deepedit/transforms.py (2)
RemapLabelsToSequentiald(87-155)NormalizeLabelsInDatasetd(158-177)
🪛 Ruff (0.14.10)
tests/apps/deepedit/test_deepedit_transforms.py
343-343: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: packaging
- GitHub Check: build-docs
🔇 Additional comments (6)
tests/apps/deepedit/test_deepedit_transforms.py (3)
28-28: LGTM!Import correctly added for new transform class.
286-317: LGTM!Test correctly verifies deterministic alphabetical ordering regardless of input dictionary order. Both label array values and label_names mapping are validated.
319-332: LGTM!Test correctly validates multiple non-background labels with alphabetical ordering.
monai/apps/deepedit/transforms.py (2)
87-155: LGTM!Transform correctly implements deterministic alphabetical label remapping. Background is explicitly handled at index 0, and non-background labels are sorted and sequentially indexed starting from 1. Comprehensive docstring with clear examples.
158-178: LGTM!Deprecated wrapper correctly maintains backward compatibility. Clear deprecation message with stacklevel=2 for proper warning attribution. Delegates functionality to RemapLabelsToSequentiald via inheritance.
monai/apps/deepedit/__init__.py (1)
12-44: LGTM!Public API correctly exports both RemapLabelsToSequentiald and deprecated NormalizeLabelsInDatasetd. Imports match all list, ensuring backward compatibility.
…label ordering bug
### Description
Rename NormalizeLabelsInDatasetd to RemapLabelsToSequentiald to better describe
its actual functionality. The old name was confusing as it suggests normalization
when it actually remaps arbitrary label values to sequential indices (0, 1, 2, 3, ...).
### Bug Fix
Fixed a bug where the order of labels in the input dictionary affected the output.
Previously, if background appeared first (e.g., `{background: 0, organ1: 1, organ2: 2}`),
the transform would skip index 1 and produce `{background: 0, organ1: 2, organ2: 3}`.
This was caused by enumerate starting at 1 for all items but skipping background
without adjusting the index. The fix excludes background from enumeration and
handles it separately.
### Changes
- Renamed NormalizeLabelsInDatasetd to RemapLabelsToSequentiald
- Fixed label ordering bug by excluding background from enumeration
- Kept NormalizeLabelsInDatasetd as deprecated alias for backward compatibility
- Enhanced documentation to clearly explain remapping behavior
- Added alphabetical sorting for deterministic output ordering
- Added tests for deprecated name warning and proper remapping
### Types of changes
- [x] Non-breaking change (fix or new feature that would not break existing functionality)
- [x] New tests added to cover the changes
Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
aymuos15
force-pushed
the
refactor/remap-labels-sequential
branch
from
e1f6041 to
d999603
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/apps/deepedit/test_deepedit_transforms.py (1)
334-348: Misleading comment but test logic is correct.Line 343's comment "# Call to trigger the warning" is misleading—the warning is triggered during instantiation at line 342, not during the call. However, the test logic correctly verifies exactly one warning is raised, so the behavior is sound.
🔎 Clarify the comment
- _ = transform(data) # Call to trigger the warning + _ = transform(data) # Invoke the transform to confirm functionality
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
monai/apps/deepedit/transforms.pytests/apps/deepedit/test_deepedit_transforms.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
tests/apps/deepedit/test_deepedit_transforms.pymonai/apps/deepedit/transforms.py
🧬 Code graph analysis (2)
tests/apps/deepedit/test_deepedit_transforms.py (1)
monai/apps/deepedit/transforms.py (2)
RemapLabelsToSequentiald(87-155)NormalizeLabelsInDatasetd(158-177)
monai/apps/deepedit/transforms.py (1)
monai/transforms/transform.py (1)
MapTransform(372-497)
🔇 Additional comments (6)
tests/apps/deepedit/test_deepedit_transforms.py (3)
28-28: LGTM!Import addition is correct and necessary for the new test methods.
286-317: LGTM!Excellent test coverage for deterministic ordering. The expected output correctly validates that alphabetical sorting produces consistent results regardless of input dictionary order.
319-332: LGTM!Good test case for multiple labels. Expected mappings are correct.
monai/apps/deepedit/transforms.py (3)
87-120: LGTM!Comprehensive documentation with clear explanation of remapping behavior, good example usage, and coverage of edge cases.
129-156: LGTM!Implementation correctly sorts non-background labels alphabetically and assigns sequential indices. Edge cases (empty label_names, missing background) are handled properly. The remapping logic is sound.
158-177: LGTM!Proper deprecation pattern with clear warning message and backward compatibility maintained.
|
#7800 also mentions NormalizeLabeld from https://github.com/Project-MONAI/MONAILabel/blob/main/monailabel/transform/pre.py That is out of scope I guess since its about a different repo. Hence, I will create a separate issue + PR for that. |
| return d | ||
|
|
||
|
|
||
| class NormalizeLabelsInDatasetd(RemapLabelsToSequentiald): |
There was a problem hiding this comment.
We should definitely mark the old name as deprecated but we should instead use the deprecated decorator for this. The deprecation is also since version 1.6.0 and will be removed in version 1.8.0
There was a problem hiding this comment.
Making this change now. Sorry I missed this. Thank you!
Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @tests/apps/deepedit/test_deepedit_transforms.py:
- Around line 334-355: The test incorrectly defines a local TestDeprecatedClass
instead of validating the real deprecated class; replace the local class with
the actual deprecated class by importing NormalizeLabelsInDatasetd and wrap it
with the deprecated decorator at test time (e.g., decorated =
deprecated(since="1.6", removed="1.8", msg_suffix="Use
`RemapLabelsToSequentiald` instead.",
version_val="1.6")(NormalizeLabelsInDatasetd)) then instantiate
decorated(keys="label", label_names=...) and run the same warnings assertions to
confirm a FutureWarning mentioning "RemapLabelsToSequentiald".
🧹 Nitpick comments (1)
monai/apps/deepedit/transforms.py (1)
134-134: Preserve input array dtype.
np.zeros(d[key].shape)defaults tofloat64. Consider preserving the input dtype for consistency.♻️ Proposed fix
- label = np.zeros(d[key].shape) + label = np.zeros(d[key].shape, dtype=d[key].dtype)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
monai/apps/deepedit/transforms.pytests/apps/deepedit/test_deepedit_transforms.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
tests/apps/deepedit/test_deepedit_transforms.pymonai/apps/deepedit/transforms.py
🧬 Code graph analysis (1)
tests/apps/deepedit/test_deepedit_transforms.py (2)
monai/apps/deepedit/transforms.py (1)
RemapLabelsToSequentiald(87-155)monai/utils/deprecate_utils.py (1)
deprecated(41-120)
🔇 Additional comments (6)
tests/apps/deepedit/test_deepedit_transforms.py (3)
28-28: LGTM!Import correctly added for the new transform.
286-317: LGTM!Test correctly validates that different input orderings produce identical, alphabetically-sorted output.
319-332: LGTM!Clean test case for multi-label mapping.
monai/apps/deepedit/transforms.py (3)
27-27: LGTM!Import correctly added for deprecation support.
136-148: LGTM!Clean implementation of alphabetical sorting with background exclusion. The
enumerate(sorted_labels, start=1)pattern correctly assigns sequential indices.
158-170: LGTM!Deprecation wrapper correctly implemented per MONAI conventions.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
monai/apps/deepedit/transforms.pytests/apps/deepedit/test_deepedit_transforms.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
tests/apps/deepedit/test_deepedit_transforms.pymonai/apps/deepedit/transforms.py
🧬 Code graph analysis (2)
tests/apps/deepedit/test_deepedit_transforms.py (2)
monai/apps/deepedit/transforms.py (2)
RemapLabelsToSequentiald(87-153)NormalizeLabelsInDatasetd(157-167)monai/utils/deprecate_utils.py (1)
deprecated(41-120)
monai/apps/deepedit/transforms.py (3)
monai/utils/deprecate_utils.py (1)
deprecated(41-120)monai/utils/module.py (2)
min_version(273-285)optional_import(315-445)monai/transforms/transform.py (1)
MapTransform(372-497)
🔇 Additional comments (6)
tests/apps/deepedit/test_deepedit_transforms.py (3)
28-28: LGTM!Import correctly adds the new public class name.
286-315: LGTM!Test correctly validates deterministic alphabetical ordering regardless of input dictionary order, addressing the bug mentioned in PR objectives.
317-329: LGTM!Test correctly validates multiple non-background labels with alphabetical ordering.
monai/apps/deepedit/transforms.py (3)
27-27: LGTM!Import correctly adds
deprecateddecorator utility.
87-153: LGTM!Implementation correctly remaps labels to sequential indices with deterministic alphabetical ordering. Docstring is comprehensive and follows Google style.
156-167: LGTM!Deprecation wrapper correctly uses
@deprecateddecorator with appropriate version parameters and inherits fromRemapLabelsToSequentialdfor backward compatibility.
- Apply black formatting to transforms.py and test file - Improve test_deprecated_name_warning docstring to explain the testing approach - Add verification that actual NormalizeLabelsInDatasetd class works - Rename test class to DeprecatedNormalizeLabels for clarity Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
aymuos15
force-pushed
the
refactor/remap-labels-sequential
branch
from
6798485 to
4916f3a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
monai/apps/deepedit/transforms.py (1)
122-127: Consider adding__init__docstring.Per coding guidelines, all definitions should have docstrings. The class-level docstring covers the Args, but an
__init__docstring would be more consistent with Google-style.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
monai/apps/deepedit/transforms.pytests/apps/deepedit/test_deepedit_transforms.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
tests/apps/deepedit/test_deepedit_transforms.pymonai/apps/deepedit/transforms.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: packaging
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: build-docs
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-os (macOS-latest)
🔇 Additional comments (8)
monai/apps/deepedit/transforms.py (4)
27-27: LGTM!Import added correctly for the
deprecateddecorator.
87-120: Well-documented class.Comprehensive docstring with Args, Example, and Note sections. Clearly explains the remapping behavior and alphabetical ordering.
129-153: LGTM!Remapping logic is correct:
- Sorts non-background labels alphabetically
- Assigns sequential indices starting at 1
- Background handled separately at index 0
- Undefined labels default to 0 via the zeroed array initialization
156-167: LGTM!Deprecated wrapper correctly uses
@deprecateddecorator with proper version info. Docstring clearly indicates the deprecation and replacement class.tests/apps/deepedit/test_deepedit_transforms.py (4)
28-28: LGTM!Import added for the new
RemapLabelsToSequentialdclass.
286-315: LGTM!Test correctly validates:
- Different input orderings produce identical output
- Alphabetical sorting is applied consistently
- Both label array and label_names are verified
317-329: LGTM!Correct validation of multiple label remapping with alphabetical ordering.
331-378: LGTM!Test validates:
- Docstring deprecation notice
- Subclass relationship
- Deprecation mechanism via simulated version
- Actual class functionality
The approach of using
version_val="1.6"to simulate triggering the warning is a practical workaround when the actual MONAI version may be < 1.6.
2 participants
Description
Rename
NormalizeLabelsInDatasetdtoRemapLabelsToSequentialdto better describe its actual functionality. The old name was confusing as it suggests normalization when it actually remaps arbitrary label values to sequential indices (0, 1, 2, 3, ...).Fixes #7800
Bug Fix
Fixed a bug where the order of labels in the input dictionary affected the output.
Previously, if background appeared first (e.g.,
{background: 0, organ1: 1, organ2: 2}), the transform would skip index 1 and produce{background: 0, organ1: 2, organ2: 3}.This was caused by enumerate starting at 1 for all items but skipping background without adjusting the index. The fix excludes background from enumeration and handles it separately.
Changes
NormalizeLabelsInDatasetdtoRemapLabelsToSequentialdNormalizeLabelsInDatasetdas deprecated alias for backward compatibilityTypes of changes
./runtests.sh --quick --unittests --disttests.