pesq calculation terminated in the middle without no error message about pesq HOT 15 CLOSED

I meant to check the buffer for "silent moments", as this may cause the program to crash due to no utterances being found.
In my case I was literally just checking if the buffer only contained numbers under a certain threshold, which is definitely not optimal. I got slightly better results with a Moving Average, but I'm positive there are better silence detection algorithms out there that I didn't have the time to look for then.

I'm going to try to brew a best-of-both-worlds solution and I'll PR it in the next couple of days. Can't guarantee to deliver it fast, as I'm currently looking for a job and working part-time, so please don't depend on me!

As a preview of what I'm thinking: I'm going to try to have multiple implementations of pesq(), so the default keeps the old behaviour, one with return values and one with exceptions. User can select which one they want on import.

from pesq import pesq
from pesq import pesq_except as pesq
from pesq import pesq_retvals as pesq

Or something like that. I'll also try to have that being an option directly in the pesq call as well: pesq(bla bla bla bla, errors={PANIC, EXCEPTION, RETURN}), with default being either panicking for backwards compatibility or exception for user-friendlyness.

from pesq.

pesq will request some heap memory during computing.
this memory size depends on the length of audio as I remembered.
you can split one long audio file into shorter-size ones and retry.
if problems remain after that, you can use https://github.com/samedii/python-pesq, which can raise exceptions.

from pesq.

Is there a plan to merge this or add a similar feature?
In fgnt/pb_bss#27 @jonashaag requested to change our code to use the fork.
I would prefer to use an updated version of this repo. Ideally from PyPI.

from pesq.

Agreed !
Maybe the owner of the fork (@samedii & @Rafagd) are planning to make a PR?

from pesq.

@samedii created this PR some months ago, where we've discussed some alternative implementations of the same fix with @ludlows, as they had a problem with the semantics and performance implications of exception throwing.

I think in the end @ludlows decided to prototype a solution in a branch, but they might not have had the time to do it.

I can add that one way you can prevent this error from happening is to check if your buffer is not filled with zeros.
That was the main cause of crashes for me.

from pesq.

Thanks for all the discussions above.

This project was created under the work of my Master Thesis.
Right Now, I have graduated and don't have enough time to prototype the new feature about exception throwing.
The following is my plan:
Firstly, I will update the current version on github to the PyPi platform.
Secondly, I will create a new branch for the features on exception throwing.

@Rafagd you can pr your zero-checking code to master branch so that I can update this change to PyPi.

just let me know if you have any concerns.

from pesq.

I am not sure, what @Rafagd meant with zero checking, but I would prefer to be as close as possible to the original implementation.
This means, no manipulation of the calculated value and if something is wrong, raise an exception.
So the user or the followup framework is responsible to handle the exception.
In my case each time, when pesq wasn't working, something on my side was wrong and after fixing pesq worked.

from pesq.

I agree about raising exception instead of outputting some NaN or zero.

from pesq.

I assume no one relies on PANIC, an exception also produces an failure of the program.
The only case where it may change user code, is when the user catches randomly all Exceptions.

Is the logic of pesq_retvals then:

def pesq_retvals(...)
    try:
        return pesq_except(...)
    except Exception:
        return np.nan

?

from pesq.

Well, currently on the C backend I'm already returning values, and then I'm checking the values in Cython to raise the correct exception. So it would be pretty much the other way around.

Ie.:

#current implementation
def pesq(...):
      [.... STUFF ....]
      cdef int res = pesq_measure(&ref_info, &deg_info, &err_info, &error_flag, &error_type);

     if res == 1: #
         raise PESQError("Invalid sampling frequency")
     if res == 2:
         raise PESQError("No utterances detected")
     if error_flag!=0: # probably kept from past implementations.
         return -1
     return err_info.mapped_mos

#future implementation
def pesq_retvals(...):
    [.... STUFF ....]
    cdef int res = pesq_measure(&ref_info, &deg_info, &err_info, &error_flag, &error_type);
    if res > 0:
        return -res-1 # -2 for invalid, -3 for no utterances
    if error_flag != 0:
        return -1 
    return err_info.mapped_mos
  
def pesq_except(...):
    cdef int res = pesq_retvals(....)
    if res == 1: #
         raise PESQError("Invalid sampling frequency")
    if res == 2:
         raise PESQError("No utterances detected")
    return res

I was thinking if it would be worth to inline the implementation to avoid the extra call.

from pesq.

I think inline could be done, but it is not nessesary. PESQ is slow enough, that it does not matter.

I feared that.
The pesq_retvals codes the errors with C style error codes.
This is unnatural in Python and numpy functions like np.mean wouldn't recognize that the calculation is completely wrong.

I personally think that using pesq_retvals can be problematic:

• It hides the actual error (Error pass silently)
• What if the user only want to suppress one exception?
• The user should never interpret the error code, for that is pesq_except much better.
• If such a function is implemented, I would expect np.nan or float('-inf'), so they break the followup code.

For the pesq_except, how about using different exception types? That would make it easier to suppress just one type of failure.

from pesq.

The user should never interpret the error code, for that is pesq_except much better.

Well, that's mainly why I had implemented it the way I did back then. What I was proposing was just to offloading the decision to the user of the library. If you really want to bypass exceptions, here, have the c-style.

Ps: Additionally, the original implementation already returns -1, in case some error happens.

If such a function is implemented, I would expect np.nan or float('-inf'), so they break the followup code.

The problem with NaN is that I couldn't express the reason why the code failed, but it is definitely doable.

What if the user only want to suppress one exception?

I was planning on improving that bit, that code was a somewhat hastly done patch to fix a problem I was having back then.

from pesq.

Double posting to link my PR, couldn't really resist to have a go at it.

I have tested using the two files provided by this repository and it is working for them. Can someone test this code on a larger base to check it for validity? Also, I have checked the code and it was already using exception to validate arguments, so this whole discussion was kinda pointless. I have exposed the option to silently return values anyway, just in case someone is interested.

Also, it would be nice if @samedii could replicate his changes from his PR on this one, just for the sake of keeping everyone's changed tagged to their names.

from pesq.

As the users of this project ,@boeddeker and @mpariente, could you give opinions on this PR

from pesq.

The code on dev branch has provided the error-handling behaviors.

from pesq.