Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

effects.trim: top_db can be negative? doc says top_db is a number > 0. #1809

Open
Qubitium opened this issue Feb 19, 2024 · 1 comment
Open
Labels
discussion Open-ended discussion for developers and users

Comments

@Qubitium
Copy link

Qubitium commented Feb 19, 2024

Describe the bug

While trying to figure out why efffects.trim is failing to trim silence I came across an old issue on same topic which had reference working code which I confirmed worked in my case of:

librosa.effects.trim(audio, top_db=-10, ref=0.0173, frame_length=512, hop_length=1)

Ref an older issue:
#1445

This code snippet actually works and solved my issue but I am scratching my head on how this works while doc is saying top_db should never be negative.

So is doc wrong or incomplete?

Expected behavior

Crash and burn? =) The documentation explicitly says top_db is float > 0

documentation:

def trim(
    y: np.ndarray,
    *,
    top_db: float = 60,
    ref: Union[float, Callable] = np.max,
    frame_length: int = 2048,
    hop_length: int = 512,
    aggregate: Callable = np.max,
) -> Tuple[np.ndarray, np.ndarray]:
    """Trim leading and trailing silence from an audio signal.

    Parameters
    ----------
    y : np.ndarray, shape=(..., n)
        Audio signal. Multi-channel is supported.
    top_db : number > 0
        The threshold (in decibels) below reference to consider as
        silence

Software versions

Latest librosa

@bmcfee
Copy link
Member

bmcfee commented Feb 19, 2024

Good catch. Negative top db might make sense or not, depending on what ref is set to. When ref is np.max, negative top db would be nonsense, since the cutoff would be above max and everything would fail. But if ref is static, i could see a case for using negative topdb to specify a margin above the noise floor that you want to exceed. It's kind of a strange way to do it, but it ought to work.

I'm not quite sure whether we should tighten the implementation to rule out this latter case, or relax the documentation to explicitly support it. What do other folks think?

@bmcfee bmcfee added the discussion Open-ended discussion for developers and users label Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Open-ended discussion for developers and users
Development

No branches or pull requests

2 participants