librosa
Warn if implicit sr of 22050 is being used
Some functions have a default sr of 22050.
I just noticed after a few days that I had been using librosa.iirt(y) instead of librosa.iirt(y, sr=sr) and therefore my spectrograms were shifted by 1 octave. If I hadn't checked it, I would never have noticed, superficially it looks correct.
Also the documentation should be updated to pass sr=sr in examples.
There should be a loud warning if sr is not explicitly passed, especially because the default value is surprising, and as such there's probably a few people with erroneous spectrograms who don't even realise.
This is actually a more subtle point than it may seem. However, I'll preface the following by agreeing that the documentation examples should be audited and corrected to include sampling rates in almost all cases. (Adding a tag for +documentation here.)
If we didn't care about API compatibility, the easiest thing to do is make sr a required, keyword-only argument with no default value. Of course, this breaks API, and is not something I think we can do until the 2.0 series because of how much existing code it would break.
The next easiest way to accomplish this would be to change default sr values to None, and then have some logic to detect if sr=None was passed in, raise a warning, and set the default. This would also work - we do this kind of thing for parameter rename deprecations already, and it's probably the way to go if we want to implement the required parameter idea above in the long term.
The hiccup here is that many of our functions accept either a time-domain representation (y, sr) or a spectrogram (or something like it, S). Most of the time, the sampling rate is required in either case, but this is not universal. Just at a quick glance, the chroma functions, poly features, and a few others have code paths that may entirely ignore sr. (Eg, calling poly_features with S=data, freq=explicit_frequencies.) In these cases, I don't think it's a mistake to call without specifying sr since it's not going to be used. However, I could imagine that the logic to detect these situations could get pretty complex, and it would probably lead to some confusing or inconsistent API choices across the entire package.
Adding a tag to 0.10.1 for the docstring example audit.
For the bigger picture, I think it deserves some more time and thought. However, I do lean toward the assumption that using default parameters is fine and should not be considered a mistake. Curious for inputs from other folks on this though.
The next easiest way to accomplish this would be to change default sr values to None, and then have some logic to detect if sr=None was passed in, raise a warning, and set the default.
We could extend this idea by introducing a context manager that sets the default sampling rate. Then at the start of e.g. main (or a snippet), users who prefer a globally set sr could do:
with default_sampling_rate(44100):
# use all kind of librosa functions without having to pass sr around
...
The context manager would set and restore the default. This would also allow for locally switching a different default sampling rate, because nested usages of the context manager would restore the old default.
And we could go as far as raising an exception if no default has been set at all. This would ensure that bugs resulting from an implicit fallback cannot happen.
Users who prefer passing around sr explicitly can obviously still do that but would be fully safe that they never accidentally get the wrong thing.
#1573 And please reconsider this too!
What's to reconsider? I have not seen much (anything?) in the way of counter-arguments to this post. As I said before, if people don't want to automatically resample on load, that's totally fine - they can use soundfile directly and bypass librosa.load.
We could extend this idea by introducing a context manager that sets the default sampling rate. Then at the start of e.g.
main(or a snippet), users who prefer a globally setsrcould do:
This seems to me like inviting a lot of trouble - I generally don't like things that invisibly manipulate global state, as it will make the resulting code much more difficult to understand by reading.
But also, this is exactly (except for the context manager detail) what the presets package is for, and we already have documentation on how to use it for exactly this purpose. I suppose presets could be extended to provide a context manager interface, and that might be a better design than what's currently there, but I think this functionality is better handled in a dedicated library than building some specific state management logic into librosa.
I generally don't like things that invisibly manipulate global state
Me neither, which is why I try to pass sr around consistently. The problem is that explicit sr passing is error prone due to the implicit fallback, because forgetting to do so produces weird results instead of an error. It would just be nice to be able to use librosa in an "explicit" mode as well that has no fallback and throws an exception when forgetting to pass-in defaulted constants.
It would just be nice to be able to use librosa in an "explicit" mode as well that has no fallback and throws an exception when forgetting to pass-in defaulted constants.
Sure, I agree with that in principle. There are a few problems with implementing this kind of thing well though, not the least of which is that the people who need it the most (ie newbies messing around with parameters they don't fully understand) are those least likely to want to use a "strict" mode.
I expect the sklearn and pytorch folks deal with this kind of thing a lot — or maybe not as much as they should! — where there are similarly magic numbers that take some expertise to use correctly (slack tradeoff in svm, epsilon parameters in batchnorm layers, etc etc etc) and the defaults are almost certainly wrong for most users outside of some very bland contexts. It doesn't seem to me like they've landed on a solution for this, other than having verbose documentation and hoping that users educate themselves. Maybe I'm wrong on this?
The other main issue with a strict-mode API is that python makes this difficult to pull off elegantly. AFAIK the only way to really do it is to stuff all defaulted kwargs with placeholders (eg None) in the signature, then detect and replace those at runtime with whatever additional logic is needed to raise warnings and exceptions. We do this kind of thing in a few places, but it's not exactly user-friendly, and it makes documentation a real mess.
Ok, after a bunch of back and forth with ye olde chatbot, I think I have a docstring scraper that more or less does this job of identifying docstring examples that rely on functions accepting sr=.
I'll roll this into a separate PR I have open for some minor docstring corrections #1827 and run the audit there.