onset_strength_multi uses nfft=2048 and hop_length=512 by default instead of preset values

[philgzl]

Correct me if I'm wrong but I believe it happens here. The default values are hard coded inside the function instead of in the list of key word arguments like usual. So when setting a preset using the presets library with e.g. sr=44100, nfft=4096 and hop_length=1024, the new default values are not used.

[bmcfee]

Thanks for catching that. I've tested it locally, and confirm that presets doesn't correctly handle this particular case.

In general, presets will not be able to detect this sort of thing. The two fixes that come to mind are:

1. refine the API in librosa to not rely on kwargs.get(...) for handling default parameters.
2. extend presets to allow you to set explicit defaults at the function level, rather than the module level.

(1) is the easiest to fix, though it comes up in a few places and will require careful thought to do correctly.

(2) might be unexpectedly tricky, and still not give you an out-of-the-box solution, but I think it's the only way to reliably work around dynamic kwargs getting.

Maybe @beasteers has some thoughts on this too?

[beasteers]

Yeah this is a problem with kw args and setting defaults via signature inspection.

One of the issues that is more of an issue is subclassing/function wrapping (where we use **kw) and I don't see an easy way around this. This is an inconvenience for instance in pumpp because say I wanted to make a pump like:

class SpecialMel(Mel):
    def __init__(self, *a, blah=10, **kw):
        self.blah = blah
        return super().__init__(*a, **kw)

We can't set the defaults for params passed to Mel.

---

Another issue is that inter module references are not updated. For example, you might expect thatt n_mels would be set as a default for librosa.core.mel_frequencies when it's called inside librosa.feature.melspectrogram, but it's not.

from presets import librosa
librosa.update(dict(n_mels=256, nfft=4096, sr=None))

y, sr = librosa.load(librosa.util.example_audio_file())
S = librosa.feature.melspectrogram(y=y, sr=sr)
# S.shape == (128, 2647) # expected 256, :/

---

I'm not sure of an automated way to address this. I would say 2 would be the best way, although 1 isn't a bad idea to do as well. We could have something like this:

librosa.update(dict(sr=44100))
librosa.scope_update({
    'core.onset_strength_multi': dict(nfft=1024)
})

Or maybe this too ?

librosa.update(dict(sr=44100, nfft=1024))
librosa.scope_update({
    'core.onset_strength_multi': dict(nfft=...) # filled with 1024
})

And then we can use kw = dict(self._func_defaults.get(...), **kw) which doesn't use function introspection so it can work even with kwargs.

[beasteers]

To bad there's no way to pass a custom **kw object that would recursively get passed through subfunctions and only unpack attributes in the function signature, but I don't think that's remotely possible/practical/trivial in Python lol

[bmcfee]

To bad there's no way to pass a custom **kw object that would recursively get passed through subfunctions and only unpack attributes in the function signature, but I don't think that's remotely possible/practical/trivial in Python lol

mir_eval does this sort of thing in its submodule-level evaluate() routines, and yeah, it's a total hack: https://github.com/craffel/mir_eval/blob/14ab13634d7e55ca3d405983f16b3dc581f1b28a/mir_eval/util.py#L876-L904

---

Digging through our codebase for fishy kwarg usage turns up the following:

get

→ grep kwargs.get *.py */*.py
display.py:    fmin = fmin * 2.0**(_kwargs.get('tuning', 0.0) / bins_per_octave)
onset.py:    n_fft = kwargs.get('n_fft', 2048)
onset.py:    hop_length = kwargs.get('hop_length', 512)
feature/spectral.py:    n_chroma = kwargs.get('n_chroma', 12)

I'm generally fine with promoting those parameters up to their containing functions in these cases; using kwargs for this was just lazy. This change would not incur any compatibility issues, so I'm fine with rolling it into 0.7.1.

The odd one out here is fmin in display, since it's coming from inside a private helper function for axis decoration. I think this can stay as is, since it should be called from specshow which already promotes the fmin parameter. The kwargs usage in this case was to promote a standard API across all axis decoration helpers.

set

→ grep kwargs.set *.py */*.py
display.py:    kwargs.setdefault('color', next(axes._get_lines.prop_cycler)['color'])
display.py:    kwargs.setdefault('cmap', cmap(data))
display.py:    kwargs.setdefault('rasterized', True)
display.py:    kwargs.setdefault('edgecolors', 'None')
display.py:    kwargs.setdefault('shading', 'flat')
onset.py:    kwargs.setdefault('pre_max', 0.03*sr//hop_length)       # 30ms
onset.py:    kwargs.setdefault('post_max', 0.00*sr//hop_length + 1)  # 0ms
onset.py:    kwargs.setdefault('pre_avg', 0.10*sr//hop_length)       # 100ms
onset.py:    kwargs.setdefault('post_avg', 0.10*sr//hop_length + 1)  # 100ms
onset.py:    kwargs.setdefault('wait', 0.03*sr//hop_length)          # 30ms
onset.py:    kwargs.setdefault('delta', 0.07)
onset.py:        kwargs.setdefault('fmax', 11025.0)
feature/spectral.py:    kwargs.setdefault('pad', False)
feature/utils.py:    kwargs.setdefault('polyorder', order)
feature/utils.py:    kwargs.setdefault('mode', 'constant')
feature/utils.py:        kwargs.setdefault('constant_values', [0])
util/_nnls.py:    kwargs.setdefault('m', A.shape[1])
util/utils.py:    kwargs.setdefault('mode', 'constant')
util/utils.py:    kwargs.setdefault('mode', 'constant')

There's quite a bit more going on here, but I the unifying theme is to propagate default values that can be directly overridden. If presets doesn't work in these cases, that's a problem with presets, and not our usage here.

Proposed solution

Fix the two cases in onset.py and one in feature/spectral.py, and punt on the rest.

Does that sound reasonable? @beasteers ?

[beasteers]

mir_eval does this sort of thing in its submodule-level evaluate() routines, and yeah, it's a total hack:

I like the idea, but it would fail in a case like this (which I don't think is more common than uncommon):

def a(a=1, b=2, c=3):
    pass

def b(d, e, f, **kw):
    return a(**kw)

b(4, 5, 6, a=3, y=8) # `y` gets passed to `a()`

So I agree that it's a bit hacky here.

(Though I must admit, I've been tempted on several occasions to implement something like that for myself.)

---

Yeah I agree that those setdefaults are not super fixable and I think it's just complexity that presets won't be able to handle.

So yep. Sounds reasonable. 👍🏻

[bmcfee]

Ok, I'll put out a PR this afternoon to fix this one up.