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

Add seek call after sf.info #942

Merged
merged 9 commits into from Aug 14, 2019
Merged

Add seek call after sf.info #942

merged 9 commits into from Aug 14, 2019

Conversation

carlthome
Copy link
Contributor

@carlthome carlthome commented Aug 7, 2019

Reference Issue

Fixes #941

What does this implement/fix? Explain your changes.

Rewinds the read position in librosa.stream in case the input is a file handle.

import librosa as lr

# Works as expected.
next(lr.stream(lr.util.example_audio_file(), 1, 1024, 512))

# Fixed by this pull request:
next(lr.stream(open(lr.util.example_audio_file(), 'rb'), 1, 1024, 512))

Any other comments?

Perhaps a unit test should be added for this as well? I'm also curious if this is really something librosa should handle or if it's actually a mistake in soundfile.info?

@bmcfee bmcfee added the bug Something doesn't work like it should label Aug 7, 2019
@bmcfee bmcfee added this to the 0.7.1 milestone Aug 7, 2019
@bmcfee bmcfee self-requested a review August 7, 2019 15:09
@bmcfee
Copy link
Member

bmcfee commented Aug 7, 2019

Perhaps a unit test should be added for this as well? I'm also curious if this is really something librosa should handle or if it's actually a mistake in soundfile.info?

I think we should have a unit test for this regardless of if it's a bug (or otherwise undocumented behavior in soundfile).

@carlthome
Copy link
Contributor Author

@bmcfee I added the code path to test_stream. Do you think it's fine to skip a with statement or explicit close call? I believe it's closed on garbage collection but not sure about pytest.

@bmcfee
Copy link
Member

bmcfee commented Aug 8, 2019

Do you think it's fine to skip a with statement or explicit close call? I believe it's closed on garbage collection but not sure about pytest.

Thanks for doing this!

I think the best way to handle setup/teardown is with a pytest fixture. Something like what's described here: https://docs.pytest.org/en/latest/fixture.html#fixture-finalization-executing-teardown-code

librosa/core/audio.py Outdated Show resolved Hide resolved
@carlthome
Copy link
Contributor Author

Believe this is ready for merge, @bmcfee and @lostanlen

librosa/core/audio.py Outdated Show resolved Hide resolved
librosa/core/audio.py Outdated Show resolved Hide resolved
Copy link
Member

@bmcfee bmcfee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @carlthome , this looks great! I had two last nitpicks, noted below.

@bmcfee
Copy link
Member

bmcfee commented Aug 14, 2019

LGTM! Thanks for all your work on this one!

@bmcfee bmcfee merged commit 39db1dd into librosa:master Aug 14, 2019
@carlthome carlthome deleted the patch-3 branch August 14, 2019 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something doesn't work like it should
Development

Successfully merging this pull request may close these issues.

File objects work with librosa.load but not librosa.stream
2 participants