fix: abstract away category indexing differences#254
Conversation
Semseg and non-semseg datasets behave differently (position-based vs. explicit indexing). Now computing index field in case position-based indexing is used. Signed-off-by: Max SCHMELLER <max.schmeller@tier4.jp>
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
There was a problem hiding this comment.
Pull request overview
This PR standardizes category indexing behavior across different T4Dataset revisions by introducing a compatibility layer that handles datasets with explicit vs. position-based category indexing.
Changes:
- Added
fix_category_table()function to compute missing category indices for datasets using position-based indexing - Updated category-to-index mappings to use the
category.indexfield instead of enumeration position - Introduced new
t4_devkit/compatibility.pymodule for dataset revision compatibility helpers
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| t4_devkit/compatibility.py | New module containing fix_category_table() to normalize category index fields across dataset revisions |
| t4_devkit/tier4.py | Applies category table fix during initialization and updates label-to-id mapping to use explicit index field |
| t4_devkit/helper/rendering.py | Updates label-to-id mapping to use explicit category index instead of enumeration position |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.annotation_dir, SchemaName.CALIBRATED_SENSOR | ||
| ) | ||
| self.category: list[Category] = load_table(self.annotation_dir, SchemaName.CATEGORY) | ||
| self.category = fix_category_table(self.category) |
There was a problem hiding this comment.
The fix_category_table() function mutates the input list in place and then returns it. This creates ambiguity about whether a new list is being assigned or the existing list is being modified. Consider either making the function purely mutating (returning None) or purely functional (creating a new list), and update the calling code accordingly.
| self.category = fix_category_table(self.category) | |
| fix_category_table(self.category) |
| """ | ||
| if any(category.index is None for category in categories): | ||
| if not all(category.index is None for category in categories): | ||
| raise ValueError("Category index is not set for some categories.") |
There was a problem hiding this comment.
The error message is generic and doesn't help users diagnose which categories have missing indices. Consider including details about which categories are affected, such as 'Category index is not set for some categories: [list of category names or positions]'.
| raise ValueError("Category index is not set for some categories.") | |
| missing_positions = [i for i, category in enumerate(categories) if category.index is None] | |
| raise ValueError( | |
| f"Category index is not set for some categories at positions: {missing_positions}" | |
| ) |
| for idx, category in enumerate(categories): | ||
| category.index = idx |
There was a problem hiding this comment.
The function mutates the Category objects in place, which may have unintended side effects if the same category objects are referenced elsewhere. Consider documenting this mutation behavior in the docstring or making the function create new Category instances to avoid side effects.
|
The discussion is still ongoing, but this PR demonstrates the problem and proposes a potential fix. |
" Signed-off-by: Max SCHMELLER <max.schmeller@tier4.jp>
Semseg and non-semseg datasets behave differently (position-based vs. explicit indexing).
This leads to mismatches when trying to visualize LiDAR segmentation data:
This PR fixes that by computing the index field in case position-based indexing is used,
and using the index field for visualization.
From what I can tell, the category index is currently only used for visualization. Relations to AnnotatedInstance etc. are done via
tokenfield and not affected by this PR.See TIER IV INTERNAL discussion for details.