Merge pull request #24 from DocGarbanzo/claude/remove-hardcoded-tub-fields-y1gqh

Remove hardcoded tub fields and silent fallbacks from TubStatistics

Author: DocGarbanzo

donkeycar/parts/tub_statistics.py

@@ -187,46 +187,35 @@ class TubStatistics(object):
     def __init__(self,
                  tub: Tub,
                  config: Optional[Any] = None,
-                 gyro_z_index: int = 1,
                  sorting_strategy: Optional[SortingStrategy] = None,
                  field_aggregations: Optional[List] = None):
         """
         Construct tub statistics calculator for tub
 
         :param tub:                 input tub
         :param config:              Config object (loads FIELD_AGGREGATIONS,
-                                    LAP_SORTING_CRITERIA)
-        :param gyro_z_index:        z coordinate in 3d gyro vector (deprecated,
-                                    use config)
+                                    LAP_SORTING_CRITERIA). Required if
+                                    field_aggregations not provided.
         :param sorting_strategy:    Optional custom sorting strategy
-                                    (backward compat)
         :param field_aggregations:  Optional list of FieldAggregationSpec or
-                                    old-style dicts (backward compat)
+                                    dicts. Required if config not provided.
+        :raises ValueError:         If neither config nor field_aggregations
+                                    provided.
         """
         self.tub = tub
 
-        # Load field aggregations with precedence:
-        # 1. Direct parameter (backward compat)
-        # 2. From config (new preferred method)
-        # 3. Default fallback
+        # Load field aggregations - require explicit configuration
         if field_aggregations is not None:
-            # Handle both old-style dicts and new FieldAggregationSpec
             self.field_aggregations = self._normalize_field_aggregations(
-                field_aggregations, gyro_z_index)
+                field_aggregations)
         elif config:
             self.field_aggregations = (
                 self._load_field_aggregations_from_config(config))
         else:
-            # Backward compatible defaults
-            self.field_aggregations = [
-                FieldAggregationSpec(
-                    field='car/gyro',
-                    output_key='gyro_z_agg',
-                    index=gyro_z_index,
-                    transform=abs,
-                    aggregation='avg'
-                )
-            ]
+            raise ValueError(
+                'TubStatistics requires either config or field_aggregations. '
+                'No silent defaults are used - configure FIELD_AGGREGATIONS '
+                'in config or pass field_aggregations directly.')
 
         # Load sorting strategy
         if sorting_strategy:
@@ -240,58 +229,45 @@ def __init__(self,
         logger.info(f'Creating TubStatistics with '
                     f'{len(self.field_aggregations)} field aggregations')
 
-    def _normalize_field_aggregations(self, field_aggregations: List,
-                                     gyro_z_index: int) -> List[
+    def _normalize_field_aggregations(self, field_aggregations: List) -> List[
                                          FieldAggregationSpec]:
-        """Convert old-style dict specs to FieldAggregationSpec."""
+        """Convert dict specs to FieldAggregationSpec.
+
+        :raises ValueError: If old-style extractor syntax is used.
+        """
         normalized = []
         for spec in field_aggregations:
             if isinstance(spec, FieldAggregationSpec):
                 normalized.append(spec)
             elif isinstance(spec, dict):
-                # Old style dict with 'extractor' and 'transform'
                 if 'extractor' in spec:
-                    # Cannot convert old-style extractor lambdas,
-                    # use gyro_z_index default
-                    logger.warning('Old-style field_aggregations dict with '
-                                 'extractor not supported. Using defaults.')
-                    normalized.append(FieldAggregationSpec(
-                        field=spec['field'],
-                        output_key=spec['output_key'],
-                        index=gyro_z_index,
-                        transform=spec.get('transform'),
-                        aggregation='avg'
-                    ))
-                else:
-                    # New style dict
-                    normalized.append(FieldAggregationSpec(
-                        field=spec['field'],
-                        output_key=spec['output_key'],
-                        index=spec.get('index'),
-                        transform=spec.get('transform'),
-                        aggregation=spec.get('aggregation', 'avg')
-                    ))
+                    raise ValueError(
+                        f'Old-style field_aggregations with "extractor" '
+                        f'not supported for field {spec.get("field", "?")}. '
+                        f'Use "index" parameter instead.')
+                normalized.append(FieldAggregationSpec(
+                    field=spec['field'],
+                    output_key=spec['output_key'],
+                    index=spec.get('index'),
+                    transform=spec.get('transform'),
+                    aggregation=spec.get('aggregation', 'avg')
+                ))
         return normalized
 
     def _load_field_aggregations_from_config(self, config) -> List[
         FieldAggregationSpec]:
-        """Load field aggregation specs from config."""
+        """Load field aggregation specs from config.
+
+        :raises ValueError: If FIELD_AGGREGATIONS not found in config.
+        """
         config_specs = getattr(config, 'FIELD_AGGREGATIONS', None)
 
         if not config_specs:
-            # Fallback to legacy GYRO_Z_INDEX
-            gyro_z_index = getattr(config, 'GYRO_Z_INDEX', 1)
-            logger.info(f'No FIELD_AGGREGATIONS in config, using '
-                       f'default gyro aggregation with index {gyro_z_index}')
-            return [
-                FieldAggregationSpec(
-                    field='car/gyro',
-                    output_key='gyro_z_agg',
-                    index=gyro_z_index,
-                    transform=abs,
-                    aggregation='avg'
-                )
-            ]
+            raise ValueError(
+                'FIELD_AGGREGATIONS not found in config. '
+                'Please define FIELD_AGGREGATIONS in your config file. '
+                'Example: FIELD_AGGREGATIONS = [{"field": "car/gyro", '
+                '"output_key": "gyro_z_agg", "index": 1, "aggregation": "avg"}]')
 
         # Convert config dicts to FieldAggregationSpec
         specs = []
@@ -317,7 +293,9 @@ def _load_sorting_strategy_from_config(self, config) -> SortingStrategy:
                        f'{[c["key"] for c in criteria]}')
             return SortingStrategy(criteria)
         else:
-            logger.info('No LAP_SORTING_CRITERIA in config, using defaults')
+            logger.info('No LAP_SORTING_CRITERIA in config, using minimal '
+                       'defaults (time, distance). Configure LAP_SORTING_CRITERIA '
+                       'in config to include custom fields like gyro_z_agg.')
             return default_lap_sorting_strategy()
 
     def generate_laptimes_from_records(self, overwrite=False):

donkeycar/pipeline/transformations.py

@@ -109,12 +109,15 @@ def get_sort_value(item):
 
 def default_lap_sorting_strategy() -> SortingStrategy:
     """
-    Create default lap sorting strategy (backward compatible).
+    Create default lap sorting strategy.
 
-    Sorts by: time, distance, gyro_z_agg (all ascending).
+    Sorts by: time and distance (all ascending).
+    Only includes fields that are always computed from lap timing.
+
+    Note: For custom field rankings (e.g., gyro_z_agg), configure
+    LAP_SORTING_CRITERIA in your config file.
     """
     return SortingStrategy([
         {'key': 'time'},
         {'key': 'distance'},
-        {'key': 'gyro_z_agg'},
     ])

donkeycar/tests/test_course_segmentation_integration.py

@@ -16,10 +16,22 @@
 
 from donkeycar.config import Config
 from donkeycar.parts.tub_v2 import Tub
-from donkeycar.parts.tub_statistics import TubStatistics
+from donkeycar.parts.tub_statistics import TubStatistics, FieldAggregationSpec
 from donkeycar.pipeline.types import TubDataset, PctMode
 
 
+# Standard field aggregation for gyro_z at index 1 (simulator convention)
+GYRO_Z_INDEX_1 = [
+    FieldAggregationSpec(
+        field='car/gyro',
+        output_key='gyro_z_agg',
+        index=1,
+        transform=abs,
+        aggregation='avg'
+    )
+]
+
+
 class TestSegmentationTrainingIntegration(unittest.TestCase):
     """Test segmentation integration with training pipeline"""
 
@@ -47,7 +59,19 @@ def _create_test_config(self):
         cfg = Config()
         cfg.USE_LAP_0 = False
         cfg.TRAIN_TEST_SPLIT = 0.8
-        cfg.GYRO_Z_INDEX = 1
+        cfg.FIELD_AGGREGATIONS = [
+            {
+                'field': 'car/gyro',
+                'output_key': 'gyro_z_agg',
+                'index': 1,
+                'aggregation': 'avg'
+            }
+        ]
+        cfg.LAP_SORTING_CRITERIA = [
+            {'key': 'time'},
+            {'key': 'distance'},
+            {'key': 'gyro_z_agg'},
+        ]
         return cfg
 
     def _create_test_tub(self):
@@ -211,7 +235,7 @@ def test_lap_mode_still_works(self):
         # Need to compute lap performance first
         tub = Tub(self.tub_path, read_only=True)
         self.open_tubs.append(tub)
-        stats = TubStatistics(tub)
+        stats = TubStatistics(tub, field_aggregations=GYRO_Z_INDEX_1)
 
         # This would normally be done by generate_laptimes_from_records
         # but we created metadata manually, so just verify it exists

donkeycar/tests/test_lap_pct_regression.py

@@ -16,8 +16,28 @@
 
 from donkeycar.config import Config
 from donkeycar.parts.tub_v2 import Tub
-from donkeycar.parts.tub_statistics import TubStatistics
+from donkeycar.parts.tub_statistics import TubStatistics, FieldAggregationSpec
 from donkeycar.pipeline.types import TubDataset, PctMode
+from donkeycar.pipeline.transformations import SortingStrategy
+
+
+# Standard field aggregation for gyro_z at index 1 (simulator convention)
+GYRO_Z_INDEX_1 = [
+    FieldAggregationSpec(
+        field='car/gyro',
+        output_key='gyro_z_agg',
+        index=1,
+        transform=abs,
+        aggregation='avg'
+    )
+]
+
+# Sorting strategy that includes time, distance, and gyro_z_agg
+FULL_SORTING_STRATEGY = SortingStrategy([
+    {'key': 'time'},
+    {'key': 'distance'},
+    {'key': 'gyro_z_agg'},
+])
 
 
 class TestLapPerformanceRegression(unittest.TestCase):
@@ -46,7 +66,14 @@ def _create_test_config(self):
         cfg = Config()
         cfg.USE_LAP_0 = False
         cfg.TRAIN_TEST_SPLIT = 0.8
-        cfg.GYRO_Z_INDEX = 1
+        cfg.FIELD_AGGREGATIONS = [
+            {
+                'field': 'car/gyro',
+                'output_key': 'gyro_z_agg',
+                'index': 1,
+                'aggregation': 'avg'
+            }
+        ]
         return cfg
 
     def test_lap_mode_unchanged(self):
@@ -115,7 +142,8 @@ def test_lap_performance_calculation_unchanged(self):
 
         tub = Tub(self.tub_path, read_only=True)
         self.open_tubs.append(tub)
-        stats = TubStatistics(tub)
+        stats = TubStatistics(tub, field_aggregations=GYRO_Z_INDEX_1,
+                              sorting_strategy=FULL_SORTING_STRATEGY)
 
         # Calculate lap performance (original method)
         session_lap_rank = stats.calculate_lap_performance()