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

Switch to alternative pesq implementation #27

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jonashaag
Copy link

It's a fork of the original pesq repo that does proper error handling (rather than simply exiting with exit(1); from C code).

cc FYI @mpariente

@jonashaag
Copy link
Author

Recommended by original author here ludlows/PESQ#12 (comment)

@boeddeker
Copy link
Member

@jonashaag Thanks for the PR.
Do you know why @ludlows does not merge that Repo?
It would be cleaner when the PyPI version will be updated.

@jonashaag
Copy link
Author

No idea, sorry

@jonashaag
Copy link
Author

Or simply switch to https://github.com/vBaiCai/python-pesq, which I have worked with in the past and didn’t cause any trouble

@boeddeker
Copy link
Member

When we added pesq, we considered three implementations:

  • Our own implementation that used subprosess
    • + Zero modifications of original code
    • - Need file system
    • + Can run in Parallel
  • ludlows
    • + Python wrapper
    • + Supports all features
    • - Modified PESQ source code
    • - Cannot run in Parallel
  • vBaiCai
    • + Python wrapper
    • - Modified PESQ source code
    • - Disabled wideband
    • - Cannot run in Parallel

Since vBaiCai does not support wideband it is not an option that I want to choose.
I am not sure in which source code it was, but in one someone changed the PESQ algorithm.
The intention was most likely to stabilize, but it changes the actual values.
This disables the ability of the comparison between papers and I want to have the score that the original PESQ produced.

We decided against our own implementation, because of the file system overhead.

@jonashaag
Copy link
Author

You're right, I just checked, the vBaiCai version has modified PESQ source.

Maybe I'll create my own wrapper... I mean the wrapper source codes look really simple

@jonashaag
Copy link
Author

What are we going to do with this problem?

@boeddeker
Copy link
Member

I was hoping that https://github.com/ludlows/python-pesq fixes this problem.
It looks like a huge modification of the source code is necessary and I don't know when they fix it.
It is problematic, that the owner does no longer use the code.

One solution would be to release our internal code that uses subprocess.
But this introduces a file writing and subprocess overhead, so I am not sure, if it is better.
Also, it needs at the moment gcc.

@ludlows
Copy link

ludlows commented Dec 14, 2020

it is a little complicated to change the code to a thread-safe version which supports multi-threads as well.
right now, I am not sure the priority among thread-safe version and the one supporting to error handling yet.

@boeddeker
Copy link
Member

As far as I know, your pesq code is thread safe, because the python GIL is not released.
Letting the code release the GIL may be difficult, because the original code uses global states.

@ludlows
Copy link

ludlows commented Dec 14, 2020

yes. but it means the code cannot running in parallel since the GIL exists.
the easy way to accelarate computing for pesq in python is subprocessing.
so handling the errors well seems more important, right?

@boeddeker
Copy link
Member

Yes, subprocess allows to execute the computation in parallel. An another alternative is multiprocessing.
I personally use most of the time MPI, where I calculate PESQ, there I don't care about the GIL.

Subprosess also handles the problem with the error handling. At least partially.

For the error handling it would be nice, when an exception is thrown. That simplifies the bug search

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.

3 participants