Impulse Response Fatigue Model, EMA Smoothing, and Acoustic Feature Extraction#2
Conversation
Khubaib8281
left a comment
There was a problem hiding this comment.
I appreciate the detailed implementation. The PR is directionally aligned with the planned Week 3 scope and introduces three useful components: EMA smoothing for stabilizing the raw fatigue signal, an impulse-response-based vocal load model with acute/chronic components and recovery estimation, and Praat/Parselmouth-based acoustic feature extraction for jitter, shimmer, HNR, and CPPS.
A few points I'd recommend addressing before merging:
-
Clarify the status of acoustic features: The features are currently extracted and stored in the session records, but they do not yet appear to influence the fatigue score, warning logic, or downstream analytics. This is fine given the stated Week 3 scope, but it would be useful to explicitly document them as auxiliary features collected for future analysis rather than implying they are currently part of the fatigue estimation model.
-
Review the
readinessmetric: Thechronic - acuteformulation is inspired by impulse-response models used in training-load research, but the interpretation of this value as vocal "readiness" is not yet validated for vocal fatigue. I would suggest either documenting this as an experimental metric or initially exposing acute/chronic load separately until the formulation is empirically validated. -
Timestamp consistency:
SessionAnalytics.add()receivests, but the impulse-response model currently usestime.monotonic()to calculate elapsed time instead. It would be good to clarify and standardize the time source so that the session timestamp and temporal load calculations remain consistent. -
Error visibility: Acoustic extraction intentionally fails gracefully, which is good for keeping monitoring uninterrupted. However,
except Exception: return {}can make debugging difficult. Consider logging the extraction failure while still returning an empty feature dictionary. -
Tests and CI: Before merging, I recommend adding focused tests for EMA behavior, impulse-response decay/accumulation, recovery ETA, session summary outputs, and the acoustic extraction failure path. These tests should also be integrated into the project's CI pipeline so that every push and pull request automatically runs the test suite and verifies that existing functionality has not regressed. Please also include a test coverage report in CI, so we can track coverage for the newly added code and ensure that the critical analytics and processing paths are adequately tested. The CI checks should pass before the PR is considered ready to merge.
One general recommendaion for future contributions: where possible, please keep PRs small and focused around a single logical change rather than combining multiple Week-level tasks into one PR. For example, EMA smoothing, impulse-response load modeling, and acoustic feature extraction could each be introduced and reviewed independently, or grouped only when they are tightly coupled.
Overall, I think the additions are a good foundation for the planned functionality. With the above points clarified and covered, particularly the distinction between currently stored acoustic features and features actively used by the fatigue model, this should be in a good position to merge.
|
Thanks for the detailed review. Addressed each point:
Also noted on keeping future PRs scoped to a single logical change, planning to split Week 3 work (baseline calibration, further session integration) into separate PRs rather than bundling them. |
Khubaib8281
left a comment
There was a problem hiding this comment.
thanks for thoughtfully addressing the review comments.
Concern:
The coverage report shows 60% overall test coverage of code, with
utils.py | 57.1%
session.py | 46.2%
audio_stream.py | 33.8%
cli.py | 0%
kindly address this issue , however the coverage for analytics.py, init.py, config.py is very solid but the mentioned files need much improvement. Once the test coverage is improved, I'll be happy to merge changes.
|
Added tests for utils.py, session.py, audio_stream.py, and cli.py, using mocking for the hardware dependent parts (microphone input, command line args) since those can't run in CI. Coverage is now at 93% overall, with utils.py and session.py at 100%. |
Khubaib8281
left a comment
There was a problem hiding this comment.
solid improvement in test coverage. PR is approved
Adds the Week 2 implementation for the design approved in the previous PR: exponential moving average smoothing for the raw fatigue score, the vocal load impulse response model with acute and chronic components and recovery time estimation, and acoustic feature extraction (jitter, shimmer, HNR, CPPS) wired into the existing processing pipeline. A minimal, backward compatible addition was made to session.py to support the new features field, full session level integration is planned for Week 3 as scoped in the design document.