61 lines
2.4 KiB
Markdown
61 lines
2.4 KiB
Markdown
|
# Phase 8: Test and Constant Cleanup - Discussion Log
|
||
|
|
|
||
|
|
> **Audit trail only.** Do not use as input to planning, research, or execution agents.
|
||
|
|
> Decisions are captured in CONTEXT.md — this log preserves the alternatives considered.
|
||
|
|
|
||
|
|
**Date:** 2026-03-27
|
||
|
|
**Phase:** 08-test-and-constant-cleanup
|
||
|
|
**Areas discussed:** Constant removal strategy, Frequency range bound, Test naming
|
||
|
|
**Mode:** Auto (all decisions auto-selected)
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Constant Removal Strategy
|
||
|
|
|
||
|
|
| Option | Description | Selected |
|
||
|
|
|--------|-------------|----------|
|
||
|
|
| Delete entirely | Remove NumLayers and GainPerLayer from config.go — bank.go already computes dynamically | ✓ |
|
||
|
|
| Deprecate with comment | Keep but mark as deprecated for backward compatibility | |
|
||
|
|
| Replace with function | Convert to a function that returns len(ClassFreqConfigs) | |
|
||
|
|
|
||
|
|
**User's choice:** [auto] Delete entirely (recommended default)
|
||
|
|
**Notes:** NewBank already computes gainPerLayer as 1.0/len(cfgs). No external callers reference either constant.
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Frequency Range Test Bound
|
||
|
|
|
||
|
|
| Option | Description | Selected |
|
||
|
|
|--------|-------------|----------|
|
||
|
|
| Dynamic validation | Derive valid range from data (positive + below Nyquist) — no magic numbers | ✓ |
|
||
|
|
| Generous static bound | Replace 1100 with e.g. 8000 Hz — simple but still hardcoded | |
|
||
|
|
| Remove range test | Delete TestFrequenciesInRange entirely — other tests cover correctness | |
|
||
|
|
|
||
|
|
**User's choice:** [auto] Dynamic validation (recommended default)
|
||
|
|
**Notes:** Key constraint: adding a class in 1100-4000 Hz range must not require editing this test.
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Test Naming
|
||
|
|
|
||
|
|
| Option | Description | Selected |
|
||
|
|
|--------|-------------|----------|
|
||
|
|
| Rename to TestClassFreqConfigsMatchAllClasses | Reflects actual invariant after NumLayers removal | ✓ |
|
||
|
|
| Keep current name | Leave as-is despite referencing deleted constant | |
|
||
|
|
| Delete test | TestClassFreqConfigsComplete already covers same invariant | |
|
||
|
|
|
||
|
|
**User's choice:** [auto] Rename to TestClassFreqConfigsMatchAllClasses (recommended default)
|
||
|
|
**Notes:** Test body already uses len() comparisons, only the name references NumLayers.
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Claude's Discretion
|
||
|
|
|
||
|
|
- Specific approach for dynamic frequency range validation (Nyquist-based vs generous static bound)
|
||
|
|
- Whether to consolidate TestClassFreqConfigsComplete with renamed test
|
||
|
|
- Whether WhisperFloor needs adjustment (likely not)
|
||
|
|
|
||
|
|
## Deferred Ideas
|
||
|
|
|
||
|
|
None — discussion stayed within phase scope.
|