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

Fixes bug in multi recogniser #496

Merged
merged 3 commits into from
Jun 9, 2021
Merged

Fixes bug in multi recogniser #496

merged 3 commits into from
Jun 9, 2021

Conversation

atruskie
Copy link
Member

@atruskie atruskie commented May 31, 2021

Fixes bug in multi recogniser

The multirecogniser was not outputting any events after the component recognizers detected events. The fix was simply to ensure NewEvents were aggregated as well as old style AcousticEvents.

Changes

Also:

  • wrote tests for the multi recognizers
  • ensured that multiple different ways of specifying sub-recognizers would work (name, soft-resolved config, absolute reference to files)
  • also ensured configs were loaded before the main analysis starts - this should produce much nicer error messages and avoid cutting audio before telling the user about the inevitable failure.
  • fixed the path to analysis programs in the test metdadata helper

Issues

Fixes #97

Visual Changes

N/A

Final Checklist

  • Assign reviewers if you have permission
  • Assign labels if you have permission
  • Link issues related to PR
  • Link any PRs or issues blocking this PR from being merged
  • Remove/Reduce warnings from edited files
  • Unit tests have been added for changes
  • Ensure CI build is passing

The multirecogniser was not outputting any events after the component recognizers detected events.  The fix was simply to ensure NewEvents were aggregated as well as old style AcousticEvents.

Also:
- wrote tests for the multi recognizers
- ensured that multiple different ways of specifying sub-recognizers would work (name, soft-resolved config, absolute reference to files)
- also ensured configs were loaded before the main analysis starts - this should produce much nicer error messages and avoid cutting audio before telling the user about the inevitable failure.
- fixed the path to analysis programs in the test metdadata helper

Fixes #97
@atruskie atruskie added the bug label May 31, 2021
@atruskie atruskie requested a review from towsey May 31, 2021 06:59
@codecov
Copy link

codecov bot commented May 31, 2021

Codecov Report

Merging #496 (2f77bcc) into master (2ffce6f) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

❗ Current head 2f77bcc differs from pull request most recent head 18e994f. Consider uploading reports for the commit 18e994f to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##           master    #496      +/-   ##
=========================================
- Coverage    0.71%   0.70%   -0.02%     
=========================================
  Files         481     481              
  Lines       48928   48973      +45     
  Branches     7678    7684       +6     
=========================================
- Hits          350     344       -6     
- Misses      48527   48579      +52     
+ Partials       51      50       -1     
Impacted Files Coverage Δ
src/Acoustics.Shared/ConfigFile/Config.cs 0.00% <0.00%> (ø)
...rc/Acoustics.Shared/Extensions/ExtensionsString.cs 0.00% <0.00%> (ø)
.../Acoustics.Shared/Extensions/FileInfoExtensions.cs 0.00% <0.00%> (ø)
src/AnalysisBase/AnalyzerConfig.cs 0.00% <0.00%> (ø)
...rams/AnalyseLongRecordings/AnalyseLongRecording.cs 0.00% <0.00%> (ø)
...alysisPrograms/Recognizers/Base/MultiRecognizer.cs 0.00% <0.00%> (ø)
src/AudioAnalysisTools/CommonParameters.cs 0.00% <ø> (ø)
...alysisTools/Tracks/MinAndMaxBandwidthParameters.cs 0.00% <0.00%> (ø)
.../AudioAnalysisTools/Tracks/OnebinTrackAlgorithm.cs 0.00% <0.00%> (ø)
.../AudioAnalysisTools/Tracks/UpwardTrackAlgorithm.cs 0.00% <0.00%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ffce6f...18e994f. Read the comment docs.

atruskie added 2 commits May 31, 2021 17:44
It acts like a Recognizer and thus must be of type RecognizerConfig.

My attempt to generalize the concept in 9c95fa5 was misguided
@towsey
Copy link
Contributor

towsey commented Jun 9, 2021

I don't know how to review this efficiently and some of the syntax I am not familiar with. However, I checked MultiRecognizer.cs and the test file, MultiRecogniserTests.cs and got the gist. You needed to update NewEvents .

@atruskie atruskie merged commit 13a6960 into master Jun 9, 2021
@atruskie atruskie deleted the multi-recogniser-fix branch June 9, 2021 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multi-recognizer - allow config files in recognizer list as well as analysis names
2 participants