Skip to content

Add low-level methods read_buffer() and write_buffer()#72

Merged
bastibe merged 3 commits intomasterfrom
issue-72
Sep 28, 2015
Merged

Add low-level methods read_buffer() and write_buffer()#72
bastibe merged 3 commits intomasterfrom
issue-72

Conversation

@mgeier
Copy link
Contributor

@mgeier mgeier commented Sep 26, 2015

Instead of NumPy arrays, these would handle plain Python buffers.

This would allow a wider range of interoperability with Python libraries that use buffers, without forcing our users to convert their buffers into NumPy arrays.

These functions should then internally be used to implement our read() and write() methods.

Apart from the number of items (not frames), these methods could take one of the strings 'short', 'int', 'float', 'double'.
They could also accept 'raw' (or 'bytes'?), which would provide an alternative solution to #25.

If we at some point split the implementation into several files, as suggested in https://github.com/bastibe/PySoundFile/issues/57#issuecomment-49281037, we could then create a low-level submodule which would have no dependencies on NumPy.

@mgeier
Copy link
Contributor Author

mgeier commented Sep 9, 2014

Alternative names would be buffer_read() and buffer_write(), respectively.
These wouldn't clutter auto-completion.

@mgeier mgeier modified the milestone: 0.7 Oct 26, 2014
@bastibe bastibe modified the milestones: 0.6.x, future Mar 4, 2015
@mgeier mgeier modified the milestone: future Mar 5, 2015
@mgeier mgeier mentioned this pull request May 27, 2015
@mgeier
Copy link
Contributor Author

mgeier commented Sep 26, 2015

I added a commit (357ef04) with a possible implementation.

This provides the 3 methods buffer_read(), buffer_read_into() and buffer_write().
In addition to buffers, buffer_write() also accepts a bytes object.

This commit also removes the hard dependency on NumPy, now it is only needed if read() or write() are used.

I didn't find a meaningful way to include sf_read_raw() and sf_write_raw() into this, but I also think it's not really important to do that.

@bastibe
Copy link
Owner

bastibe commented Sep 26, 2015

Looks good. I don't quite like how this adds yet another layer of abstraction betwee write and the actual CFFI call, but that is not a high price to pay.

I am worried about performance, though. The numpy import might incur a big overhead on every read/write. Did you check if this has any measurable performance impact?

Also, I think we should keep numpy as a dependency. For most people, pysoundfile is not useful without numpy. Can you think of any other way to handle this?

@mgeier
Copy link
Contributor Author

mgeier commented Sep 27, 2015

I am worried about performance, though. The numpy import might incur a big overhead on every read/write. Did you check if this has any measurable performance impact?

I did not measure this, but importing an already imported module should only add the overhead of a (successful) dictionary lookup of sys.modules['numpy'].
I'm quite sure that's negligible since it doesn't happen in a tight loop.
In the worst case it will happen once per block in block-based processing and I daresay the additional dictionary lookup will not be a problem.

An alternative (but not strictly equivalent) approach would be to try to import numpy at module scope and check for an ImportError.
Then we could set an internal flag and raise an error if NumPy would be needed but is not available.

But I think that's much worse, because a user might have NumPy installed but might not want to use it (e.g. to avoid the load time), which wouldn't be possible with the alternative approach.
I think it would also make the code uglier.

Also, I think we should keep numpy as a dependency. For most people, pysoundfile is not useful without numpy. Can you think of any other way to handle this?

Hmmm, I only see 3 possible kinds of dependency:

  • hard dependency with install_requires
  • optional dependency with extras_require (which is my suggestion)
  • no dependency at all

Is there another way?

If we keep it as hard dependency, people without NumPy will not be able to pip install PySoundFile, which would make this whole PR more or less useless.

OTOH, installing NumPy with pip will not work on most systems anyway (at least that's my experience). And even if it works, it will most likely be a inferior installation (with regards to the usage of optimized libraries) compared to an installation with a package manager.

I think most users will use a Python system which already has NumPy installed (like Anaconda) or they should use some other package manager to install NumPy anyway.
In the rare cases where they actually want to install with pip, they can use pip install PySoundFile[numpy].

For the Debian package, we could change the dependency from Depends to Recommends, which would mean in a default setting, NumPy would be installed automatically.

@mgeier mgeier modified the milestones: 0.8.0, future Sep 27, 2015
@bastibe
Copy link
Owner

bastibe commented Sep 28, 2015

As far as I know, numpy is working on wheels. We might need to revisit this once the Python packaging ecosystem switches over to wheels ;-)

bastibe added a commit that referenced this pull request Sep 28, 2015
Add low-level methods read_buffer() and write_buffer()
@bastibe bastibe merged commit c36e5a5 into master Sep 28, 2015
@mgeier mgeier deleted the issue-72 branch September 28, 2015 11:35
@mgeier
Copy link
Contributor Author

mgeier commented Sep 28, 2015

Thanks for merging!

I had a quick look and it looks like there are only wheels for OSX: https://pypi.python.org/pypi/numpy

We could make the dependencies OS-dependent, but I don't know if that's really a good idea.

@bastibe
Copy link
Owner

bastibe commented Sep 28, 2015

No hurry. At the moment, I agree with you that numpy is probably best installed using a package manager like conda, brew, or apt. But in a few years, who knows, maybe installing numpy won't be a problem any more, and a dependency here or there might do more good than harm.

@mgeier
Copy link
Contributor Author

mgeier commented Nov 11, 2015

Just for the record:

The Debian package changed its NumPy dependency to Recommends, 0.8.1 is now available in "sid (unstable)": https://packages.debian.org/sid/python3-soundfile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants