Re: [Discuss-gnuradio] Some misconceptions about the "peak_detector2" block

[Frank Fu]

I’ve also been looking for an appropriate fix for peak_detector2.  When I 
review this thread and the issue tracker, I’m uncertain how the block is 
supposed to behave.  I think most of the developers have looked at the 
documentation in the header file, and have tried to make fixes in accordance 
with it.  Specifically, that the peak search should only be restricted to the 
range within the look_ahead parameter.  If so, then Achilleas is very close to 
an appropriate fix, needing only some additional calls to set_output_multiple 
to prevent hanging. The block would also work fine with a sine wave, as long as 
the look ahead value was appropriate, like half the period. 

As mentioned previously, the QA test may not necessarily be useful, given that 
the input signal is much smaller than the window.  Perhaps the test file can 
also be modified to get better results. I’m willing to contribute to a fix and 
help make the peak detector block more stable, but I would have to know more 
about how the block should behave.  Any comments or insights are appreciated.

Frank Fu


-------
From:   Tom Rondeau
Subject:        Re: [Discuss-gnuradio] Some misconceptions about the 
"peak_detector2" block
Date:   Mon, 13 Apr 2015 16:54:47 -0400
Achilleas,

I think you've completely failed to understand the issue from my perspective. I 
do NOT disagree that there is a bug in the code. I also do NOT disagree that 
most of what you've tried to do in the rewrite is the correct way to rethink 
the block. What I have a problem with is that you've provided me with a fix 
that breaks applications that used to run fine, including the QA test.

As for the applications, there's a really simple test you can perform. Apply 
the peak detector to a sine wave. In the current code, it finds the peak of the 
sine wave. With the new version, it does not just find that as the peak, but it 
outputs as though it's found the peak for every length of the look-ahead value. 
This could be a valid design choice in a peak detector where given a window, 
always emit the highest value in the window. That is not what this block is 
supposed to do, nor is the new block behaving consistently in this manner.

As for the QA test, the current tests presents a vector to the block and the 
block finds the correct peak. With the new code, it doesn't complete. It just 
hangs. While it is true that stream-based blocks in GNU Radio expect a 
continuous stream of data, any block that simply fails to complete its 
processing when the rest of the flowgraph is done is a bug. Instead, we have 
hooks like set_output_multiple and overloading the forecast function that help 
us work with the scheduler to make sure everyone gets the right amount of data 
they require. In this case, you make a good argument that the block should look 
beyond it's current window to see if the max is in fact reached. If that's the 
case, then we need to have the block tell the scheduler this. If less data is 
passed because there is no more data to process and the flowgraph is shutting 
down, this block too must shut down. It will then do so without providing the 
right answer. So the QA test will fail -- but it will complete and report 
failure in the data.

Please understand the above points when reworking your fix.

Thanks,

Tom
------
On Fri, Apr 10, 2015 at 4:22 PM, Achilleas Anastasopoulos <address@hidden> 
wrote:
Hi all,

recently there has been some discussion regarding the peak_detector2 block, 
both in the github/gnuradio (pull request  404) as well as in the issue tracker 
(issue 783).

It is now well accepted that this block is buggy: there are cases the work 
function returns -1, which is a bug (see issue 783 on how to recreate this bug).

I believe however that there is a DEEPER misconception about how this block 
works/should work that has resulted in some frustration on what an appropriate  
fix should be. 
In particular there is an insistence that an appropriate bug fix should pass 
the qa_test of this block and it should be [in the spirit] of the existing 
algorithm. 
In the following I will explain why passing the qa_test is a consequence of the 
buggy behaviour  of this block and NOT its feature.
In addition I will suggest what a proper behaviour of this block should be, so 
that others who may want to write their own version of a peak detector find it 
useful.


--------

So the peak_detector block is very reasonable in its conception and its name is 
very informative and appropriate. It works as follows:

Reads the input and keeps track of a running average  (through a single-pole 
iir filter)

When the current input crosses a  threshold (= average * a user-defined factor) 
upwards the block enters a search state, where it looks for the maximum value 
of the input over a window of user-defined length.

This is clearly the intended behaviour of the block according to the 
documentation (I don't know who the original author is...).

----------