-
Notifications
You must be signed in to change notification settings - Fork 12
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
Issue451 diy recognizer feedback #459
Conversation
…riodicity Issue #451 Also write a new method to convert an array of real values to a string.
Issue #451 Add feedback about filtering events on syllable duration, bandwidth, sideband activity etc.
Issue #451 Add info concerning combining events and also add more to determination of side-band activity.
…ilters Issue #451 In particular provide more info on side-band activity filters. Also temporarily comment out calls to method that writes the results.csv file because it is not working.
Issue #451 These recognizers are either not needed or are unusable and would be extremely difficult to resurrect. Their development was discontinued around 2015 when they were in an embryonic state.
Issue #451 1: Add some booleans into the config file to control event filtering. 2: Edit the Call Recognizer Guide to reflect recent changes in the names of config parameters. 3: Remove a unit test for a discontinued call recognizer.
Issue #451 Rewrite the event detection/post-processing algorithm so that post-processing steps are performed for each level of decibel detection threshold. The guide.md file has been edited to explain this change and to make it consistent with changes to the config parameter names.
Codecov Report
@@ Coverage Diff @@
## master #459 +/- ##
==========================================
- Coverage 37.79% 0.70% -37.09%
==========================================
Files 482 470 -12
Lines 69114 48626 -20488
Branches 8062 7662 -400
==========================================
- Hits 26120 344 -25776
- Misses 42904 48232 +5328
+ Partials 90 50 -40
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not all convinced post-processing should be done per decibel threshold. If the purpose of multiple decibel threshold is to detect the same event at varying intensities, then they should be flattened into one layer. If the purpose is to detect different events, then they should be different profiles.
I actually think any event detected in the same spot in multiple threshold settings, should be flattened, and only the highest decibel event kept.
If the post-processing should be completely agnostic to algorithm used as well. What of algorithms that do not use decibel thresholds?
But assuming you have better reasons than you have explained here, these changes are still problematic.
- Changing parameters constitutes a breaking contract
- There are numerous code quality issues
- the include multiple changes unrelated to Providing users of Call Recognizers with more diagnostic feedback #451
- including the deletion of recognisers
- changing of post-processing events
docs/guides/generic_recognizers.md
Outdated
|
||
[!code-yaml[post_processing_filtering](./Ecosounds.NinoxBoobook.yml?start=34&end=62&highlight=20- "Post Processing: filtering")] | ||
|
||
Use the parameter `Duration` to filter out events that are too long or short. | ||
This filter removes events whose duration lies outside three standard deviations (SDs) of an expected value. | ||
|
||
- `FilterOnDuration` should be set true if you want to filter events on their duration, otherwise false. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This FilterOnDuration
switch is not in the example config... and also it is verbose. We should remove the concept entirely.
If a user does not want to filter on duration then they should just not provide any parameters
docs/guides/generic_recognizers.md
Outdated
Add a post processing section to you config file by adding the `PostProcessing` parameter and indenting the sub-parameters. | ||
### [PostProcessing](xref:AudioAnalysisTools.Events.Types.EventPostProcessing.PostProcessingConfig) | ||
|
||
Post-processing of events is performed after event detection. However it is important to understand that post-processing is performed once for each of the DecibelThresholds. As an example: suppose you have three decibel thresholds (6, 9 and 12 dB is a typical set of values) in each of two profiles. All the events detected at threshold 6 dB (by both profiles) will be collected together and subjected to the post processing steps. Typically some or all of the events may fail to be accepted as "true" events based on your post-processing parameters. Then all the events detected at 9 dB will be collected and independently subjected to post-processing. Then, likewise, all events detected at the 12 dB threshold will be post-processed. In other words, one round of post-processing is performed for each decibel threshold. This sequence of multiple post-processing steps gives rise to one or more temporally nested events. Think of them as Russion doll events! The final post-processing step is to remove all but the longest duration event in any nested set of events. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add line break please - just as you would for code... one long line makes diffs difficult to read
I won't mark other lines with this issue, but it applies to all the changes you made
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have inserted many line breaks.
docs/guides/generic_recognizers.md
Outdated
|
||
[!code-yaml[post_processing](./Ecosounds.NinoxBoobook.yml#L34-L34 "Post Processing")] | ||
|
||
Post processing is optional. You may just want to combine or filter component events in your own code. | ||
Post processing is optional - you may decide to combine or filter the "raw" events using code you have written yourself. To add a post-processing section to your config file, insert the `PostProcessing` parameter and indent the sub-parameters. There are five post-processing possibilities each of which you may choose to use or not. Note that the post-processing steps are performed in this order which cannot be changed by the user: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Post processing is optional - you may decide to combine or filter the "raw" events using code you have written yourself. To add a post-processing section to your config file, insert the `PostProcessing` parameter and indent the sub-parameters. There are five post-processing possibilities each of which you may choose to use or not. Note that the post-processing steps are performed in this order which cannot be changed by the user: | |
Post processing is optional - you may decide to combine or filter the "raw" events using code you have written yourself. To add a post-processing section to your config file, insert the `PostProcessing` parameter and indent the sub-parameters. Each of the post-processing possibilities are optional. Note that the post-processing steps are performed in this order which cannot be changed by the user: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have done a lot of work here to shift unnecessary documentation to other guides
docs/guides/generic_recognizers.md
Outdated
|
||
Unlike overlapping events, if you want to combine a group of events (like syllables) that are near each other but not | ||
overlapping, then make use of the `SyllableSequence` parameter. | ||
|
||
[!code-yaml[post_processing_combining_syllables](./Ecosounds.NinoxBoobook.yml?start=34&end=51&highlight=10- "Post Processing: Combining syllables")] | ||
|
||
Set `CombinePossibleSyllableSequence` true where you want to combine possible syllable sequences. A typical example is | ||
a sequence of chirps in a honeyeater call. | ||
Combining syllable sequences is enabled by setting `CombinePossibleSyllableSequence` true. A typical example is a sequence of chirps in a honeyeater call. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Combining syllable sequences is enabled by setting `CombinePossibleSyllableSequence` true. A typical example is a sequence of chirps in a honeyeater call. | |
Combining syllable sequences is enabled by setting `CombinePossibleSyllableSequence` to `true`. A typical example is a sequence of chirps in a honeyeater call. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed these where they occur.
docs/guides/generic_recognizers.md
Outdated
Use the parameter `Bandwidth` to filter out events whose bandwidth is too small or large. | ||
This filter removes events whose bandwidth lies outside three standard deviations (SDs) of an expected value. | ||
|
||
- `FilterOnBandwidth` should be set true if you want to filter events on their bandwidth, otherwise false. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto, remove docs and concept
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
// The following line does it all BUT it does not allow for feedback to the user. | ||
//var filteredEvents = events.Where(ev => ((SpectralEvent)ev).EventDurationSeconds >= minimumDurationSeconds && ((SpectralEvent)ev).EventDurationSeconds <= maximumDurationSeconds).ToList(); | ||
|
||
var filteredEvents = new List<EventCommon>(); | ||
|
||
var count = 0; | ||
foreach (var ev in events) | ||
{ | ||
count++; | ||
var duration = ((SpectralEvent)ev).EventDurationSeconds; | ||
if ((duration > minimumDurationSeconds) && (duration < maximumDurationSeconds)) | ||
{ | ||
Log.Debug($" Event{count} accepted: Actual duration = {duration:F3}s"); | ||
filteredEvents.Add(ev); | ||
} | ||
else | ||
{ | ||
Log.Debug($" Event{count} rejected: Actual duration = {duration:F3}s"); | ||
continue; | ||
} | ||
} | ||
|
||
return filteredEvents; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As with the others, an incorrect assertion, revert the change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A new extension method has been written to take care of this.
string strArray = DataTools.Array2String(actualPeriodSeconds.ToArray()); | ||
Log.Debug($" Event{count} actual periods: {strArray}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use
audio-analysis/src/Acoustics.Shared/Extensions/EnumerableExtensions.cs
Lines 307 to 319 in 5baf2d9
public static string Join<T>(this IEnumerable<T> items, string delimiter = " ") | |
{ | |
var result = new StringBuilder(); | |
foreach (var item in items) | |
{ | |
result.Append(item); | |
result.Append(delimiter); | |
} | |
// return one delimiter length less because we always add a delimiter on the end | |
return result.ToString(0, result.Length - delimiter.Length); | |
} | |
} |
string strArray = DataTools.Array2String(actualPeriodSeconds.ToArray()); | |
Log.Debug($" Event{count} actual periods: {strArray}"); | |
Log.Debug($" Event{count} actual periods: {actualPeriodSeconds.Join(",")}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New extension method has been written.
/// <summary> | ||
/// Gets or sets a value indicating Whether or not to filter events on their duration. | ||
/// </summary> | ||
public bool FilterOnDuration { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// <summary> | |
/// Gets or sets a value indicating Whether or not to filter events on their duration. | |
/// </summary> | |
public bool FilterOnDuration { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Property removed.
/// <summary> | ||
/// Gets or sets a value indicating Whether or not to filter events on their band-width. | ||
/// </summary> | ||
public bool FilterOnBandwidth { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// <summary> | |
/// Gets or sets a value indicating Whether or not to filter events on their band-width. | |
/// </summary> | |
public bool FilterOnBandwidth { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Property removed.
public static string Array2String(double[] array) | ||
{ | ||
StringBuilder sb = new StringBuilder(); | ||
for (int i = 0; i < array.Length; i++) | ||
{ | ||
sb.Append(array[i].ToString("F3") + ", "); | ||
} | ||
|
||
return sb.ToString(); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public static string Array2String(double[] array) | |
{ | |
StringBuilder sb = new StringBuilder(); | |
for (int i = 0; i < array.Length; i++) | |
{ | |
sb.Append(array[i].ToString("F3") + ", "); | |
} | |
return sb.ToString(); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New extension method written.
Issue #451 Final changes to the use of post-processing parameters in config files.
Issue #451 Changes to config.yml files and to generic recognizer tests to accommodate changes to post-processing parameters.
Issue #451 Major changes were to allow some parameter values to be nullable to avoid the use of booleans.
Issue #451 Remove an unnecessary line inserted into a unit test.
Issue #451 All these changes are to do with post-processing of acoustic events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I can see you've made some changes - thankyou.
However, there are still multiple issues with the guide. Since this is such a big PR, I only have the option of accepting or rejecting it as a whole, and on those grounds alone I'm requiring more changes.
None of these changes are hard, but if they're not done in this contribution, then it represents work I'll need to do which I don't presently have time for.
Major issues:
- still not enough line breaks (this includes line breaks you've removed from the master copy)
- over explanation of config file parameters. Any detailed explanation of a parameter should be in the docs for it's config class, not in this guide
- various syntax errors,
- with the
NOTE:
quotes (line break needed) - with bullet lists (line break needed before the start of the list
- with the
docs/guides/generic_recognizers.md
Outdated
profile in the `Profiles` list. A config file may target more than one syllable or acoustic event, in that case there | ||
would be profile for each target syllable or acoustic event. | ||
Each algorithm is designed to detect a syllable type. Thus to make a generic recognizer there should be at least one (1) | ||
profile in the `Profiles` list. A config file may target more than one syllable or acoustic event, in which case there will be profile for each target syllable or acoustic event. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
profile in the `Profiles` list. A config file may target more than one syllable or acoustic event, in which case there will be profile for each target syllable or acoustic event. | |
profile in the `Profiles` list. A config file may target more than one syllable or acoustic event, in which case there | |
will be a profile for each target syllable or acoustic event. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'a' has been inserted
docs/guides/generic_recognizers.md
Outdated
Post-processing of events is performed after event detection. Post-processing is performed once for each of the DecibelThresholds. As an example: suppose you have three decibel thresholds (6, 9 and 12 dB is a typical set of values) in each of two profiles. There will be three rounds of post-processing: | ||
- All the events detected at threshold 6 dB (by both profiles) will be collected together and subjected to the post-processing steps. Typically some or all of the events may fail to be accepted as "true" events based on your post-processing parameters. | ||
- Next all the events detected at 9 dB will be collected and independently subjected to post-processing. | ||
- Next all events detected at the 12 dB threshold will be post-processed. | ||
|
||
This sequence of multiple post-processing steps gives rise to one or more temporally nested events. Think of them as Russion doll events! The final post-processing step is to remove all but the longest duration event in any nested set of events. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Post-processing of events is performed after event detection. Post-processing is performed once for each of the DecibelThresholds. As an example: suppose you have three decibel thresholds (6, 9 and 12 dB is a typical set of values) in each of two profiles. There will be three rounds of post-processing: | |
- All the events detected at threshold 6 dB (by both profiles) will be collected together and subjected to the post-processing steps. Typically some or all of the events may fail to be accepted as "true" events based on your post-processing parameters. | |
- Next all the events detected at 9 dB will be collected and independently subjected to post-processing. | |
- Next all events detected at the 12 dB threshold will be post-processed. | |
This sequence of multiple post-processing steps gives rise to one or more temporally nested events. Think of them as Russion doll events! The final post-processing step is to remove all but the longest duration event in any nested set of events. | |
Post-processing of events is performed after event detection. Post-processing is performed once for each of the DecibelThresholds. | |
As an example: suppose you have three decibel thresholds (6, 9 and 12 dB is a typical set of values) in each of two profiles. | |
There will be three rounds of post-processing: | |
- All the events detected at threshold 6 dB (by both profiles) will be collected together and subjected to the | |
post-processing steps. Typically some or all of the events may fail to be accepted as "true" events based on your | |
post-processing parameters. | |
- Next all the events detected at 9 dB will be collected and independently subjected to post-processing. | |
- Next all events detected at the 12 dB threshold will be post-processed. | |
This sequence of multiple post-processing steps gives rise to one or more temporally nested events. Think of them as Russion doll events! | |
The final post-processing step is to remove all but the longest duration event in any nested set of events. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have rewritten parts of this but not clear what the issue was/is.
src/AnalysisConfigFiles/RecognizerConfigFiles/Towsey.NinoxBoobook.yml
Outdated
Show resolved
Hide resolved
// Add a spacer for easier reading of the debug output. | ||
Log.Debug($" "); | ||
Log.Debug($"Event count BEFORE removing enclosed events = {postEvents.Count}."); | ||
results.NewEvents = CompositeEvent.RemoveEnclosedEvents(postEvents); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment remains unanswered?
// Add a spacer for easier reading of the debug output. | ||
Log.Debug($" "); | ||
Log.Debug($"Event count BEFORE removing enclosed events = {postEvents.Count}."); | ||
results.NewEvents = CompositeEvent.RemoveEnclosedEvents(postEvents); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I understand, this is not flattening nested events, or even flattening Russian dolls, rather this is removing any event that is completely overlapped in time/frequency bounds by another event covering the same area.
If my new understanding is correct, then no change is required, other than updating the log message.
The terminology nested is wrong here since there is no parent-child relationship. Hence the confusion.
Issue #451 Changes to documentation involved shifted some documentation to other guides and to class summaries.
Also added a safe overload for the new JoinFormatted method
Issue #451 A few further changes to make a parameter available to the user with changes accordingly to the documentation.
…utEcoacoustics/audio-analysis into Issue451_DIYRecognizerFeedback
Issue #451 Fix conflicts
Also fixed the example config file and added aside styling for breakout text
Issue #451 Made some final tweaks to the guide.
Changes to the call detection algorithm.
What is the purpose of this PR?
To incorporate changes into master.To change the post-processing so that it works per-decibel threshold. See #451
Changes
Two major changes:
Issues
None as yet.
Visual Changes
The changes output more information about the effect of parameter values on the acceptance or rejection of events. THis will make it easier for the user to "tune" parameters.
Final Checklist