Make drum one-shot fadeout configurable and add contract tests#22
Make drum one-shot fadeout configurable and add contract tests#22SaltProphet wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a configurable apply_fadeout toggle to drum one-shot generation and introduces tests to prevent signature drift and validate output behavior, fitting into FORGE’s app.py processor + module-level wrapper API pattern.
Changes:
- Added
apply_fadeout: bool = TruetoFORGEAudioProcessor.generate_drum_oneshotsand theapp.generate_drum_oneshotswrapper, and threaded the flag through. - Guarded the existing tail-taper (fade-out) logic behind the new flag.
- Added a unit “contract” test for signature/annotation/default stability and an integration test asserting fadeout reduces tail energy.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| app.py | Adds apply_fadeout to drum one-shot APIs and conditionally applies the fade-out. |
| tests/unit/test_audio_processing.py | Adds a signature contract test for apply_fadeout on processor + wrapper. |
| tests/integration/test_app_quality.py | Adds an integration test validating fadeout changes tail energy. |
| @pytest.mark.unit | ||
| class TestDrumOneShotContract: | ||
| """Test drum one-shot signatures and options.""" | ||
|
|
||
| def test_generate_drum_oneshots_signatures_include_apply_fadeout(self): | ||
| """Ensure both APIs expose apply_fadeout with stable default.""" | ||
| processor_signature = inspect.signature(FORGEAudioProcessor.generate_drum_oneshots) | ||
| wrapper_signature = inspect.signature(generate_drum_oneshots) | ||
|
|
||
| processor_param = processor_signature.parameters["apply_fadeout"] | ||
| wrapper_param = wrapper_signature.parameters["apply_fadeout"] | ||
|
|
||
| assert processor_param.default is True | ||
| assert wrapper_param.default is True | ||
| assert processor_param.annotation is bool | ||
| assert wrapper_param.annotation is bool | ||
|
|
There was a problem hiding this comment.
This contract test only checks the app.generate_drum_oneshots wrapper, but the repo also exposes a legacy compatibility wrapper in forgev1.generate_drum_oneshots (currently without apply_fadeout). If forgev1.py is still supported as a public entry point, it should be updated to accept/forward apply_fadeout (default True) and included in this signature contract to prevent API drift across wrappers.
| start_sample = int(onset_time * sr) | ||
| end_sample = int((onset_time + duration) * sr) | ||
| oneshot_audio = y[start_sample:end_sample] | ||
|
|
||
| # Apply fade out | ||
| fade_samples = int(0.01 * sr) | ||
| if len(oneshot_audio) > fade_samples: | ||
| fade = np.linspace(1, 0, fade_samples) | ||
| oneshot_audio[-fade_samples:] *= fade | ||
| # Optionally apply fade out | ||
| if apply_fadeout: | ||
| fade_samples = int(0.01 * sr) | ||
| if len(oneshot_audio) > fade_samples: | ||
| fade = np.linspace(1, 0, fade_samples) | ||
| oneshot_audio[-fade_samples:] *= fade |
There was a problem hiding this comment.
oneshot_audio = y[start_sample:end_sample] returns a NumPy view, and the in-place fade (oneshot_audio[-fade_samples:] *= fade) will mutate the underlying y buffer. This can unintentionally affect subsequent one-shot slices if ranges overlap. Consider copying the slice before applying the fade (e.g., make oneshot_audio an independent array) to keep the source audio immutable during extraction.
Motivation
Description
apply_fadeout: bool = TruetoFORGEAudioProcessor.generate_drum_oneshotsand the module-levelgenerate_drum_oneshotswrapper inapp.pyand passed the flag through end-to-end.apply_fadeoutis true by guarding the existing taper logic inside the processor (tapering the last N milliseconds when enabled).TestDrumOneShotContractintests/unit/test_audio_processing.pyto assert both APIs exposeapply_fadeoutwithboolannotation and a default ofTrue.test_drum_oneshots_apply_fadeout_toggleintests/integration/test_app_quality.pythat runs generation withapply_fadeout=Falseandapply_fadeout=Trueand asserts that fade-enabled outputs reduce tail energy versus non-faded outputs.Testing
pytest -q -o addopts='' tests/unit/test_audio_processing.py tests/integration/test_app_quality.pyand observed all tests pass:15 passed, 2 warnings.pytestfailed due to repositorypytest.iniadding coverage options not available in this environment, so-o addopts=''was used to override those options and run the suite successfully.Codex Task