Work on whistle algorithm #477
Conversation
Issue #470 Endeavour to fix error where event was found outside search frequency band. I believe this error was caused by the rounding of the search bounds from Hertz to a frequency bin. I have endeavored to tighten up the conversion of Hertz bound to frequency bin. Also added amore components to the unit test for whistle events. Also added two lines to write spectrogram results for visualization. This is a temporary fix - to be removed later.
Codecov Report
@@ Coverage Diff @@
## master #477 +/- ##
===========================================
+ Coverage 0.71% 38.53% +37.81%
===========================================
Files 481 482 +1
Lines 48928 67647 +18719
Branches 7678 7901 +223
===========================================
+ Hits 350 26067 +25717
+ Misses 48527 41489 -7038
- Partials 51 91 +40
Continue to review full report at Codecov.
|
Issue #470 Fixed broken whistle unit test that resulted from changing the name of two variables. Fixed Australasian Pipet broken test that resulted from previous changes as noted by Truskinger. In addition, added two new parameters, SearchbandMinHertz and SearchbandMaxHertz, because the parameters MinHertz and MaxHertz were being used to specify two different kinds of bound, that is the min and max bounds of the search band in which to find events and as the min and max bounds of an actual event. This issue has been fixed for the Whistle and the Whip generic events but I request Truskinger to determine if he is happy with how this has been done before proceeding with other generic events where this problem also occurs.
|
I have fixed two broken unit tests. The whistle test was broken due to introduction of two new variables to remove an ambiguous dual use of the variables MinHertz and MaxHertz. |
|
I haven't reviewed this properly yet, but some notes:
My understanding is the existing If I've misunderstood, then we should set up a call? |
#470 Add more detailed summary to variables SearchbandMinHertzand and SearchbandMaxHertz.
|
Sorry - I am having a bit of trouble getting internet connection here so I upload even when not finished. I rewritten the summaries for the two new variables and hopefully they explain what is intended. Min/MaxHertz are event parameters where as SearchbandMin/MaxHertz are algorithm parameters. I am not sure that I have placed them in the best position. The new interval library looks good. I commented out the two validation lines to get the new variables working. I am not sure where the most logical place is to put the new variables. They will be common to most algorithms but not all. The Blob algorithm and the Harmonics algorithm would not currently need the distinction. However I am thinking to write a new Blob2 event recognizer which would detect blob events of limited bandwidth anywhere in a wide search band. This would be a better way to do your Bell Bird recognizer. If this does not make the issues clearer then we will have to have a call. But it is not easy here on French Island. We are leaving FI on Thursday and driving to Albury on Friday. |
|
Ok the new documentation makes sense. To summarize: the new definition for Follow up questions:
For reference here are all the places where
All these uses were consistent. Thus, this change is a partially implemented suggestion to change all uses? Can I suggest that no change to the names are needed currently? Or as another suggestion, leave |
|
We're communicating offline with bad internet connections so I'm including a summary of our convsation here for prosperity:
My response: Like I said in the issue I think the simplest thing is not change the meaning of [Min|Max]Hertz simply because it's usage is currently consistent and widespread. Let's treat [Min|Max]Hertz as search bounds - it's just the simplest way forward, even if their current meaning was not your original intended meaning.
As you pointed out, most algorithms at the moment simply default to using the search range as the bounds of the event. When more pertinent information is available (e.g. whistle algorithm defaults to three bins) then more precise bounds are reported. I think rather than changing [Min|Max]Hertz to mean event bandwidth limit we should keep the definition as search limits and for algorithms that need event bandwidth limits we should add a new parameter: MinEventBandwidthHertz and MaxEventBandwidthHertz (or with an interval, EventSizeHertz: "[100, 300]") I agree that [Min|Max]Hertz probably is the best term to describe the event bandwidth limit, however, that ship has sailed.
Can you link me to it? I can add more context then.
Yes I absolutely want to make new algorithms. We should come up with a more descriptive name though. Blob2 can work as a beta/dev name until the semantics and differences are clear. To summarize, you did:
I'm suggesting:
Because that change is far smaller |
Issue #477 Removed two variables that set min and max search bounds. Instead added comments to existing variable MinHertz and MaxHertz that indicate these variables in fact are used to set in and max search bounds. i.e. they are not the bounds of an acoustic event except where the discovered events occupy the entire search bandwidth.
|
I have removed the names of the two search bound variables I created and in addition added comments to the existing Min/MaxHertz variables indicating that they are actually functioning as search bounds. |
| yield return this.MinHertz.ValidateNotNull(nameof(this.MinHertz)); | ||
| yield return this.MaxHertz.ValidateNotNull(nameof(this.MaxHertz)); | ||
| //yield return this.MinHertz.ValidateNotNull(nameof(this.MinHertz)); | ||
| //yield return this.MaxHertz.ValidateNotNull(nameof(this.MaxHertz)); |
There was a problem hiding this comment.
Is there a reason this validation is disabled?
Revert change or delete validation (and explain).
| if (maxSearchBin > binCount - 6) | ||
| { | ||
| maxSearchBin = binCount - 6; |
There was a problem hiding this comment.
What is this magic number 6? Is that the "sideband"? Extract to a constant?
| var bandIntensity = ((sonogramData[tf, bin - 1] * 0.5) + sonogramData[tf, bin] + (sonogramData[tf, bin + 1] * 0.5)) / 2.0; | ||
| var topSidebandIntensity = (sonogramData[tf, bin + 3] + sonogramData[tf, bin + 4] + sonogramData[tf, bin + 5]) / 3.0; | ||
| var netAmplitude = 0.0; | ||
| if (bin < 4) | ||
| if (bin < 5) |
| /// <summary> | ||
| /// Gets or sets a value indicating the minimum Hertz value of the search band. | ||
| /// </summary> | ||
| public int? SearchbandMinHertz { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets a value indicating the maximum Hertz value of the search band. | ||
| /// </summary> | ||
| public int? SearchbandMaxHertz { get; set; } | ||
|
|
There was a problem hiding this comment.
These do not appear to be used. They're redundant? Remove?
| int minSearchBin = (int)Math.Floor(parameters.SearchbandMinHertz.Value / binWidth); | ||
| if (minSearchBin < 1) | ||
| { | ||
| minSearchBin = 1; | ||
| } | ||
|
|
||
| // set top search bin allowing for the top sideband. | ||
| int maxSearchBin = (int)Math.Floor(parameters.SearchbandMaxHertz.Value / binWidth) - 1; |
There was a problem hiding this comment.
The incorrect parameters SearchbandMinHertz and SearchbandMaxHertz are still being used here
There was a problem hiding this comment.
I have made the corrections as requested. I have also made an extensive class summary for the OneBinTrackAlgorithm class i.e. the class used to find whistles. I hope this will be helpful in understanding the algorithm and explaining the "magic numbers". In fact all the track algorithms require that a lot of thought be given to the choice of sample rate, frame size and frame step. The typical values are not always appropriate depending on the target call.
Issue #447 I have also written an extensive class summary for OneBinTrackAlgorithm i.e. the algorithm used to find whistles.
Issue #470 Endeavour to fix error where event was found outside search frequency band. I believe this error was caused by the rounding of the search bounds from Hertz to a frequency bin. I have endeavored to tighten up the conversion of Hertz bound to frequency bin. Also added amore components to the unit test for whistle events.
Also added two lines to write spectrogram results for visualization. This is a temporary fix - to be removed later.
Incorporate repair to whistle algorithm
Changes
Main change is the how the search bounds in Hertz are converted to bin search bounds.
Also added more components to the unit test.
Issues
It is possible that this "error" could occur again in situation where the lower search bound is very close to an actual whistle event. I did duplicate this error with the original code but have not been able to duplicate it with the revised code - so hopefully the error is fixed. If the error occurs again I believe it is due to the frequency conversion as described above and is not a serious error.
Visual Changes
Two lines were added to the unit test in order to produce a debug image. These can be deleted when the debug spectrogram gets written to locatable folder.
Fixes #470