fix(transforms3d): remap CubicSymmetry labels - #433
Merged
Conversation
Reviewer's GuideCubicSymmetry now only emits a semantic label-mapping event for rotoreflection operations (indices 24–47), leaving pure rotations (indices 0–23) label-preserving, and a new regression test verifies that semantic mask labels are remapped only when a rotoreflection is realized. Sequence diagram for CubicSymmetry semantic label-mapping event gatingsequenceDiagram
participant Pipeline
participant CubicSymmetry
participant SemanticMappingObserver
Pipeline->>CubicSymmetry: sample_parameters(data)
CubicSymmetry-->>Pipeline: { index, volume_shape }
Pipeline->>CubicSymmetry: _get_label_transform_name(index)
CubicSymmetry-->>Pipeline: "CubicSymmetry" or None
alt [index >= 24]
Pipeline->>SemanticMappingObserver: CubicSymmetry
SemanticMappingObserver-->>Pipeline: semantic_labels_remapped
else [index < 24]
Pipeline-->>SemanticMappingObserver: no_event
SemanticMappingObserver-->>Pipeline: labels_preserved
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider replacing the magic index thresholds (0..47 and >=24) with named constants or an enum that documents which indices are rotations vs rotoreflections to make the mapping logic more self-explanatory.
- In
_get_label_transform_name, you may want to guard against out-of-range indices (e.g., >47) or unexpected types more explicitly so that future changes tosample_parametersdon’t silently skip semantic mappings.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider replacing the magic index thresholds (0..47 and >=24) with named constants or an enum that documents which indices are rotations vs rotoreflections to make the mapping logic more self-explanatory.
- In `_get_label_transform_name`, you may want to guard against out-of-range indices (e.g., >47) or unexpected types more explicitly so that future changes to `sample_parameters` don’t silently skip semantic mappings.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0887265160
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| meanings and must not alter semantic labels. | ||
| """ | ||
| index = params.get("index") | ||
| return "CubicSymmetry" if isinstance(index, int) and index >= 24 else None |
There was a problem hiding this comment.
When KeypointParams.label_mapping contains a CubicSymmetry mapping and an index in 24..47 is sampled, this hook is also consumed by DualTransform._apply_label_mapping_to_keypoints, whose default implementation swaps entire keypoint rows. That is inconsistent with the established 3D reflection behavior in Flip3D, which preserves transformed coordinate-row order and remaps only the encoded label column; consequently a CubicSymmetry rotoreflection can unexpectedly reorder a user's keypoints. Override the keypoint mapping as Flip3D does, or decouple this semantic-mask event from keypoint mapping.
Useful? React with 👍 / 👎.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Antigravity Review
Findings
1. CRITICAL CORRECTNESS BUG: Keypoint Spatial Coordinate Corruption during Label Mapping
- File:
albumentations/augmentations/transforms3d/transforms.py - Line: 1922 (class
CubicSymmetry) - Severity: High
- Description:
CubicSymmetrysupports 3D keypoints, and when an orientation-reversing rotoreflection (indices 24–47) is applied, keypoint label mapping is triggered if configured. However,CubicSymmetrydoes not override_apply_label_mapping_to_keypoints. As a result, it falls back to the default implementation inDualTransform._apply_label_mapping_to_keypoints(defined inalbumentations/core/transforms_interface.py). The default implementation calls_swap_keypoint_rows_by_labels, which swaps entire coordinate rows in the keypoints array. Because the physical 3D coordinates have already been correctly transformed, swapping coordinate rows based on label matching corrupts the spatial locations of the keypoints. - Remediation: Override
_apply_label_mapping_to_keypointsinCubicSymmetryto rename/remap the label column values in-place without changing the coordinate row indices, identical to the implementation inFlip3D:def _apply_label_mapping_to_keypoints(self, keypoints: np.ndarray, **params: Any) -> np.ndarray: processor = self.get_processor("keypoints") transform_name = self._get_label_transform_name(**params) if ( not isinstance(processor, KeypointsProcessor) or not processor.params.label_fields or keypoints.size == 0 or transform_name is None ): return keypoints field_mappings = processor.encoded_label_mappings.get(transform_name) if not field_mappings: return keypoints result = keypoints.copy() for label_offset, label_field in enumerate(processor.params.label_fields): mapping = field_mappings.get(label_field) column_index = NUM_KEYPOINTS_COLUMNS_IN_ALBUMENTATIONS + label_offset if not mapping or column_index >= keypoints.shape[1]: continue source_values = keypoints[:, column_index] for source_label, target_label in mapping.items(): result[source_values == source_label, column_index] = target_label return result
2. MISSING TESTS: Lack of Keypoint Label Mapping Verification
- File:
tests/test_semantic_mask_label_mapping.py - Severity: Medium
- Description: While the pull request introduces
test_semantic_mask_label_mapping_follows_realized_cubic_symmetry_operationto verifymask3dsemantic label mapping, there are no tests verifying keypoint label mapping forCubicSymmetry. - Remediation: Add a parameterized test similar to
test_flip3d_odd_reflections_remap_keypoint_labels_without_reordering_rowsto verify that orientation-reversing rotoreflections underCubicSymmetrycorrectly remap keypoint labels in-place without corrupting/swapping coordinate rows.
3. MISSING DOCUMENTATION: Missing Label Mapping Details in Class Docstring
- File:
albumentations/augmentations/transforms3d/transforms.py - Line: 1922
- Severity: Low
- Description: The docstring for
CubicSymmetrylacks documentation regarding the newly introduced label mapping behavior. Users will not know that rotoreflections (indices 24–47) emit theCubicSymmetrylabel-mapping event, whereas pure rotations (indices 0–23) do not. - Remediation: Append a note to the docstring of
CubicSymmetrydescribing this behavior for parity withFlip3D's docstring.
4. ROBUSTNESS: Strict Type Verification for index Parameter
- File:
albumentations/augmentations/transforms3d/transforms.py - Line: 2011
- Severity: Low
- Description: In
_get_label_transform_name,isinstance(index, int)is used. Ifindexis passed as a numpy integer (e.g.,np.int64) during manual execution or custom parameter pipelines, this check can returnFalse. - Remediation: Robustly check for integer types using
isinstance(index, (int, np.integer))or cast safely after verification.
Residual Risk
- Test Execution: Because we did not run the full testing suite via pytest, we cannot guarantee there are no indirect regressions.
- Code Standards & Linting: Code quality and style checks (such as Ruff and Mypy) were not run on the patched files to verify strict type correctness and linter compatibility.
There was a problem hiding this comment.
Antigravity Review
There are no actionable findings. The pull request is demonstrably correct, safe, backwards-compatible, and conforms to all repository design principles, correctness contracts, and testing conventions.
Summary of Strengths
-
Design & Correctness:
- Restricting
CubicSymmetrysemantic-label remapping only to rotoreflections (index$\ge 24$ ) while keeping pure rotations (index$0 \dots 23$ ) label-preserving is correct. Pure rotations preserve volume chirality/handedness, so semantic labels must remain unchanged. - Elevating
_apply_label_mapping_to_keypointstoTransform3Dis an excellent design choice. It prevents code duplication betweenFlip3DandCubicSymmetry, and ensures that 3D transforms correctly remap label columns in-place within keypoint coordinate rows rather than swapping entire coordinate rows (which is the behavior for 2D keypoint regression).
- Restricting
-
Quality of Implementation:
- The simultaneous label remappings (e.g.,
2: 3, 3: 2) are correctly implemented using source array copies, preventing double-overwriting. - Checked types and boundaries are handled safely.
- The simultaneous label remappings (e.g.,
-
Thorough Coverage:
- Robust test coverage was added to verify both semantic mask mapping and keypoint label mapping under both parity-preserving (rotation) and parity-changing (rotoreflection) operations.
-
docs/maintaining/correctness-contracts.mdwas updated with clear documentation on the 3D Orientation Label Mappings contract.
Material Residual Risks
- Operational & Test Execution Verification: Because of the strict instruction prohibiting the execution of shell tools and test runners (
pytest,quality_gate.py), we could not run the test suite or verify the static type checker (mypy) results on the modified/added code. - Performance Verification: The performance improvements (e.g., cubic symmetry composite routes runtime reduction) reported in the PR description could not be independently verified via benchmarks.
There was a problem hiding this comment.
Antigravity Review
No actionable correctness bugs, security issues, behavioral regressions, maintainability risks, or missing tests/documentation were identified in this pull request. The implementation is highly robust, correct, elegant, and completely adheres to the project's established conventions, styling, and correctness contracts.
Summary of Strengths & Verifications
-
Design Cleanliness: Consolidating
_apply_label_mapping_to_keypointswithin theTransform3Dbase class is an excellent design choice. It removes repetitive logic from subclasses and ensures consistent row-aligned label mapping across all 3D geometric augmentations. -
Simultaneous Label Swaps: The keypoint label-mapping loop evaluates conditions against a copy of the original (unmodified) keypoint array while writing to the result array. This perfectly handles swap label dependencies (e.g.
2: 3, 3: 2) simultaneously and correctly. -
Mathematical Integrity: Limit checking the
CubicSymmetrylabel-mapping event to rotoreflections (index$\ge 24$ ) ensures that class identities are preserved during pure rotations (indices$0..23$ ), which is mathematically correct. -
Test Coverage: Added tests in both
tests/test_semantic_mask_label_mapping.pyandtests/transforms3d/test_transforms.pyare thorough and correctly parameterized. They comprehensively cover index$0$ and index$24$ cases, asserting coordinate transformations and label mappings with high precision.
Material Residual Test or Operational Risk
-
Custom Subclass Alignment Risk
Custom third-party or future internal 3D transforms subclassingTransform3Dwill now automatically inherit the row-aligned keypoint label mapping behavior instead of the row-swapping behavior used in 2D dual transforms. While this is explicitly documented in the updateddocs/maintaining/correctness-contracts.md, it represents an operational paradigm change for 3D augmentations that developers must be aware of when authoring custom transforms. -
Symmetry Group Index Ordering Dependency
The mapping checkindex >= 24assumes that rotoreflections are strictly grouped at indices$24$ to$47$ . While this currently aligns perfectly with the underlying functional module (f3d.transform_cube), any future re-ordering or optimization of the 48 cubic symmetry group elements in the functional layer will break this assumption. This risk is heavily mitigated by the added unit tests.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Fixes #432.
CubicSymmetryemits theCubicSymmetrymapping event only for realized rotoreflections (indices24..47).Pure rotations (
0..23) preserve semantic labels.For
Transform3Dtransforms that emit a mapping event:mask3dand everymask3dalias undergo geometry followed by a simultaneous class-ID remap.KeypointParams.label_mappingchanges label-field values in the same rows that already contain the transformed XYZcoordinates. It never swaps complete keypoint rows, preserving a manual annotation and instance alignment.
The keypoint behavior is centralized in
Transform3D, soFlip3DandCubicSymmetryshare the invariant. Themaintainer correctness-contract documentation records the target, row-order, alias, and instance-binding rules.
Tests and validation:
uv run pytest -q tests/transforms3d/test_transforms.py— 443 passeduv run pytest -q tests/test_semantic_mask_label_mapping.py— 39 passeduv run pytest -q tests/test_instance_binding.py— 125 passeduv run python tools/quality_gate.py fast— passed, including mypy, Pyrefly, 1,276 contract tests, and pre-commitPublic Compose benchmark, forced rotoreflection,
(32, 64, 64, 1)volume (median microseconds/call; original PR →this commit):
78.72 → 78.06; 1,024 keypoints175.09 → 177.4285.85 → 85.36; 1,024 keypoints196.35 → 196.77