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

Remove "out" from blocks() #213

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

mgeier
Copy link
Contributor

@mgeier mgeier commented Oct 25, 2017

This is a continuation of PR #209.

@bastibe
Copy link
Owner

bastibe commented Oct 25, 2017

Thank you for the pull request!

In principle, I agree that out should be removed, if only to simplify things. However, we have historically been very reluctant to mess with peoples' workflows, and removing out would be a regression. At the very least, we should deprecate out first, and then remove it as a second step (i.e. keep this pull request open, or merge it into a 0.10.x branch).

Do you have performance metrics whether out is actually useful in real world usage? I have a feeling that any performance benefits from out are insignificant in comparison with the time we spend in libsndfile. Can you think of any other benefits of out beyond performance?

Come to think of it, this line of reasoning might indicate a general removal of out from all reading functions. I guess we should think about this before 1.0.

@mgeier
Copy link
Contributor Author

mgeier commented Nov 14, 2017

removing out would be a regression

Yes, but I have the suspicion that zero people are using this feature and even if they do, it's a nicely noisy breakage and can be easily fixed by switching to SoundFile.read() with its out parameter.

But I'm also OK with a deprecation if that's what you want.

Do you have performance metrics whether out is actually useful in real world usage?

Nope.

Can you think of any other benefits of out beyond performance?

This is only indirectly relevant to performance (if at all).

The main point is that a user might have some already allocated memory. For whatever reason (possibly "performance" in some meaning of the word).
And with the out parameter we give them a way to use their memory directly, without allocating new memory and without unnecessarily copying data around.

... might indicate a general removal of out from all reading functions

Please don't.
I think this is a useful feature (see the reasoning above) and it cannot be implemented by a user given just the public API.

Removing out from blocks() is no problem because this can be easily re-implemented, but only if at least one function (in this case the SoundFile.read() method) still has the out argument.

Strictly speaking, my argument isn't quite as strong as I'd like it to be, because this functionality could still be implemented by users using SoundFile.buffer_read_into().
But I still think it would be useful to be able to stay in NumPy world for this without having to juggle with buffers.

@bastibe
Copy link
Owner

bastibe commented Nov 16, 2017

That's a reasonable argument, and I concur on all major points.

That said, I do think we should deprecate out first before deleting it (or did we only introduce it in the recent pull request? I forget).

Could you go into a bit more detail for why you would use out if it's not performance? When would it be advantageous to use pre-allocated memory where it is not done for performance reasons. The only thing I can come up with are situations where you are working with memory-mapped numpy arrays, or otherwise rely on external not-in-memory arrays. Is this what you have in mind?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants