Config

[zg1in]

Summary

Describe the main changes in this PR:

• Configurations for basic, processor
• Readme for scripts
• Small changes

---

Code Quality Checklist

Before requesting review, ensure:

• All CI/CD pipelines pass
• Code includes docstrings and type hints
• I added new tests where necessary
• I ran make tree to update the README.md

---

Additional Notes

• Anything else reviewers should know?
• Attach outputs, plots, logs, or GIFs here.

[NGierden]

@claude-review

[claude]

Claude finished @NGierden's task in 3m 2s —— View job

---

Code Review

• Gather PR diff and read changed files
• Review definitions.py changes
• Review sensor_preprocessor.py changes
• Review app.py changes
• Review remaining files
• Post findings

---

Overall the refactor is a good structural improvement — moving method selection out of PreprocessorConfig and into BasicConfig is cleaner, and separating FilteringMethod from VelocityEstimationMethod fixes a semantic misclassification. A few issues need attention before merge.

---

Bugs / Correctness

1. AttributeError on unrecognized enum values — sensor_preprocessor.py:117, 131, 153

The three strategy-selection blocks fall through to logger.warning(...) without assigning self._drift_removal, self._filtering, or self._velocity_estimation. These are type annotations only (not assignments), so any subsequent call to filter() or reset() will raise AttributeError. This silently misconfigures the system at runtime instead of failing fast. Should raise ValueError immediately:

else:
    raise ValueError(f"Unrecognized drift_removal_method: {self._basic_config.drift_removal_method}")

Fix this →

2. FilteringMethod.KALMAN has no handler — definitions.py:47, sensor_preprocessor.py:120-131

FilteringMethod declares KALMAN = auto() but __init_strategies__ has no branch for it — it falls through to the warning-only else, leaving self._filtering unset. Either add a KALMAN handler or remove the enum member until implemented.

3. time_difference clamping removed — sensor_preprocessor.py:81-82

The old code clamped time_difference = 0.01 after the gap detection. The new code calls reset() but then uses the original (>1.0) time_difference in the filter call. A 1+ second step may cause numerical issues in the IIR/state-space filters. Consider restoring the clamp after reset:

if time_difference > 1.0:
    self.reset()
    time_difference = 0.01  # restore clamping

---

Design Issues

4. __init_strategies__ naming — sensor_preprocessor.py:106

__name__()__ (double underscore prefix and suffix) is Python's convention for magic/dunder methods. A private helper should use a single leading underscore: _init_strategies. The current name will confuse readers and tools expecting it to be a special method.

Fix this →

5. BasicConfig passed to SensorPreprocessor — sensor_preprocessor.py:49

SensorPreprocessor now receives the full BasicConfig including left_limb_plot, left_limb_reverse, read_from_imu, read_data_from_path, etc. — all fields irrelevant to preprocessing. This couples unrelated concerns. A dedicated config slice (or keeping a separate PreprocessorConfig with just the method selections) would be cleaner, especially since PreprocessorConfig still exists as a class.

6. NotchConfig default center_freq_hz=0.0 — definitions.py:135

A 0 Hz center frequency is physically meaningless for a notch filter. The old code required explicit values, preventing accidental use of an unconfigured notch. The new default allows silent instantiation with a degenerate configuration. Consider keeping it required or using a sentinel that forces intentional setting.

---

Docstring / Documentation

7. Stale docstring — sensor_preprocessor.py:52

Still says :param PreprocessorConfig config: Preprocessor configuration. but the parameter is now basic_config: BasicConfig.

8. Stale module docstring — sensor_preprocessor.py:3-7

Still lists "four strategies for velocity estimation" and names SogifllVelocityEstimation as a velocity estimation strategy. SOGI is now correctly a filtering strategy. The count and list need updating.

---

Minor / Nits

9. __main__.py regression — plots no longer shown

BasicConfig(filtered=True) uses defaults left_limb_plot=False, right_limb_plot=False. The old standalone test script had plot=True hardcoded. Running python -m hip_controller now shows no live plots silently. Consider passing left_limb_plot=True, right_limb_plot=True here or noting this in the script.

10. Duplicated StrEnum backport — conftest.py:8-12

The backport is now defined in both definitions.py and conftest.py. Consider centralising it (e.g. in a utils/compat.py) and importing from there in both places.

11. LowPassFilterConfig default coupling — definitions.py:118-120

cut_off_frequency_rad_per_sec: float = BasicConfig.cut_off_freq_low_pass_rad_per_sec works, but the value (80.0) now controls the default for all LowPassFilterConfig instances (drift removal, velocity estimation LP, etc.) through PreprocessorConfig. The cut-off for each use-case may differ; make the coupling intentional by keeping explicit values per usage in PreprocessorConfig.

---

[NGierden]

See claude comments