Address review comments: SPARC, SQUIM, PPG, SpeechBrain#470
Conversation
…B revision - SPARC decode/convert: create Audio from waveform tensor directly, no dangling filepath to deleted tempdir - SQUIM: move model.to(device) before loop (was inside loop) - PPG: rename _extract_ppg_segments → extract_ppg_segments (public), _to_frame_major_posteriorgram → to_frame_major_posteriorgram (public), export from __init__.py, update tutorials and tests - SpeechBrain SER: add revision to cache key, pass revision to hf_hub_download, use yaml.safe_load instead of string splitting Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several enhancements and bug fixes across the audio processing tasks. Key updates include adding model revision support and safe YAML parsing for SpeechBrain SER, making PPG utility functions public, and fixing a bug in SPARC feature extraction where Audio objects were incorrectly linked to temporary file paths. Additionally, Torchaudio-Squim feature extraction was optimized by moving model loading outside the processing loop. However, a significant issue was identified in the test suite: several test functions were renamed without an underscore after the test prefix, which will prevent pytest from discovering and running them.
|
|
||
| def test_to_frame_major_posteriorgram_3d() -> None: | ||
| """Test _to_frame_major_posteriorgram with a (1, phonemes, frames) input.""" | ||
| def testto_frame_major_posteriorgram_3d() -> None: |
There was a problem hiding this comment.
The test function name is missing an underscore after the test prefix. This will prevent pytest from discovering and running this test by default.
| def testto_frame_major_posteriorgram_3d() -> None: | |
| def test_to_frame_major_posteriorgram_3d() -> None: |
|
|
||
| def test_to_frame_major_posteriorgram_2d() -> None: | ||
| """Test _to_frame_major_posteriorgram with a (phonemes, frames) input.""" | ||
| def testto_frame_major_posteriorgram_2d() -> None: |
There was a problem hiding this comment.
The test function name is missing an underscore after the test prefix. This will prevent pytest from discovering and running this test by default.
| def testto_frame_major_posteriorgram_2d() -> None: | |
| def test_to_frame_major_posteriorgram_2d() -> None: |
|
|
||
| def test_to_frame_major_posteriorgram_already_frame_major() -> None: | ||
| """Test _to_frame_major_posteriorgram when input is already (frames, phonemes).""" | ||
| def testto_frame_major_posteriorgram_already_frame_major() -> None: |
There was a problem hiding this comment.
The test function name is missing an underscore after the test prefix. This will prevent pytest from discovering and running this test by default.
| def testto_frame_major_posteriorgram_already_frame_major() -> None: | |
| def test_to_frame_major_posteriorgram_already_frame_major() -> None: |
|
|
||
|
|
||
| def test_to_frame_major_posteriorgram_too_few_dims() -> None: | ||
| def testto_frame_major_posteriorgram_too_few_dims() -> None: |
There was a problem hiding this comment.
The test function name is missing an underscore after the test prefix. This will prevent pytest from discovering and running this test by default.
| def testto_frame_major_posteriorgram_too_few_dims() -> None: | |
| def test_to_frame_major_posteriorgram_too_few_dims() -> None: |
|
|
||
|
|
||
| def test_extract_ppg_segments_basic() -> None: | ||
| def testextract_ppg_segments_basic() -> None: |
There was a problem hiding this comment.
The test function name is missing an underscore after the test prefix. This will prevent pytest from discovering and running this test by default.
| def testextract_ppg_segments_basic() -> None: | |
| def test_extract_ppg_segments_basic() -> None: |
|
|
||
|
|
||
| def test_extract_ppg_segments_single_phoneme() -> None: | ||
| def testextract_ppg_segments_single_phoneme() -> None: |
There was a problem hiding this comment.
The test function name is missing an underscore after the test prefix. This will prevent pytest from discovering and running this test by default.
| def testextract_ppg_segments_single_phoneme() -> None: | |
| def test_extract_ppg_segments_single_phoneme() -> None: |
|
|
||
|
|
||
| def test_extract_ppg_segments_empty() -> None: | ||
| def testextract_ppg_segments_empty() -> None: |
There was a problem hiding this comment.
The test function name is missing an underscore after the test prefix. This will prevent pytest from discovering and running this test by default.
| def testextract_ppg_segments_empty() -> None: | |
| def test_extract_ppg_segments_empty() -> None: |
SpeechBrain hyperparams.yaml uses custom YAML tags (!new:, !ref, etc.) that yaml.safe_load cannot handle. Use a dedicated SafeLoader subclass with a multi-constructor that ignores unknown tags. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
7 test functions were named testXxx instead of test_Xxx, making them invisible to pytest discovery. Now all 14 PPG/segment tests are properly discovered and pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 participant
Summary
Addresses Gemini code review comments from PRs #463, #466, #468, #469.
Fixes
.to(device)moved before loop (was per-iteration)extract_ppg_segmentsandto_frame_major_posteriorgrammade public (tutorials were using private_variants)hf_hub_downloaduses revision, YAML parsing withyaml.safe_loadinstead of fragile string splittingAll tests pass (10 PPG + full pre-commit).
🤖 Generated with Claude Code
Version
Published prerelease version:
1.3.1-alpha.16Changelog
🐛 Bug Fix
Authors: 1