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

librosa.load returns wrongly-typed int object #1748

Closed
tarepan opened this issue Sep 6, 2023 · 5 comments · Fixed by #1789
Closed

librosa.load returns wrongly-typed int object #1748

tarepan opened this issue Sep 6, 2023 · 5 comments · Fixed by #1789
Assignees
Labels
API change Does this change the behavior of existing API? good for beginners Are you new here? These issues are for you!
Milestone

Comments

@tarepan
Copy link
Contributor

tarepan commented Sep 6, 2023

Describe the bug
librosa.load returns int object instead of float.

To Reproduce

import librosa

import librosa
wave, sr = librosa.load("test.wav", sr=None, mono=True)
print(type(sr))
# <class 'int'>

Expected behavior
Typing of .load says that it returns float.
https://github.com/librosa/librosa/blob/09e4a622456bbad314e2e6d5c75879f56d728b9b/librosa/core/audio.py#L66C1-L66C31
So, the function should return float object.

Software versions

Linux-5.15.109+-x86_64-with-glibc2.35
Python 3.10.12 (main, Jun 11 2023, 05:26:28) [GCC 11.4.0]
NumPy 1.23.5
SciPy 1.10.1
librosa 0.10.1
INSTALLED VERSIONS
------------------
python: 3.10.12 (main, Jun 11 2023, 05:26:28) [GCC 11.4.0]

librosa: 0.10.1

audioread: 3.0.0
numpy: 1.23.5
scipy: 1.10.1
sklearn: 1.2.2
joblib: 1.3.2
decorator: 4.4.2
numba: 0.56.4
soundfile: 0.12.1
pooch: v1.7.0
soxr: 0.3.6
typing_extensions: installed, no version number available
lazy_loader: installed, no version number available
msgpack: 1.0.5

numpydoc: None
sphinx: 5.0.2
sphinx_rtd_theme: None
matplotlib: 3.7.1
sphinx_multiversion: None
sphinx_gallery: None
mir_eval: None
ipython: None
sphinxcontrib.rsvgconverter: None
pytest: 7.4.0
pytest_mpl: None
pytest_cov: None
samplerate: None
resampy: None
presets: None
packaging: 23.1```
@bmcfee bmcfee added the API change Does this change the behavior of existing API? label Sep 6, 2023
@bmcfee
Copy link
Member

bmcfee commented Sep 6, 2023

Thanks for catching this! It's a rather subtle thing, as the value and type of sr will depend on which codec path is taken (audioread vs soundfile), which resampling backend is used, etc.

However, there are a couple of things that should be fixed here:

  1. the default sr has value 22050 (int) and type Optional[float]. We should make this 22050.0.
  2. We should explicitly cast sr to float on return.

These are pretty easy fixes, and I don't anticipate any downstream effects.

@bmcfee bmcfee added the good for beginners Are you new here? These issues are for you! label Sep 6, 2023
@bmcfee bmcfee added this to the 0.10.2 milestone Sep 6, 2023
@tarepan
Copy link
Contributor Author

tarepan commented Sep 6, 2023

I don't anticipate any downstream effects.

I have some concern.
I notice this bug when I use float specific method (float.is_integer()).
Integer also have int-specific methods (method list).
Semantically this is subtle thing, but practically this could have some downstream effects (of course, very minor case).

I totally agree with your bug-fix plan, but we should emphasis backward-incompatibility notice in release note / change log.

@bmcfee
Copy link
Member

bmcfee commented Sep 6, 2023

Ah, to clarify, I meant unintended consequences for the rest of the library, not for downstream code.

@tarepan
Copy link
Contributor Author

tarepan commented Sep 8, 2023

Oh, I misunderstanded it, now I got it.

Can I make a pull request with your bug-fix plan?

@bmcfee
Copy link
Member

bmcfee commented Sep 8, 2023

Sure, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API change Does this change the behavior of existing API? good for beginners Are you new here? These issues are for you!
2 participants