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

Issue #469 rescaling of plot graphs #499

Merged
merged 3 commits into from
Jun 16, 2021

Conversation

towsey
Copy link
Contributor

@towsey towsey commented Jun 13, 2021

Rescaling of plots

Rescales plots before rendering them in generic recognisers. Fixes #469

Changes

Wrote a unit test to demonstrate that the rescaling of plots is working.

Issues

None. Except that also change the FFT window to Hanning in MultiRecognizer.cs.
That may or maynot cause a change somewhere.

Final Checklist

  • [Anthony ] 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
  • [ Yes] Unit tests have been added for changes
  • Ensure CI build is passing

Issue #469 Also remove two unused methods from DataTools.cs.
@codecov
Copy link

codecov bot commented Jun 13, 2021

Codecov Report

Merging #499 (7a33e9c) into master (025eafc) will increase coverage by 1.58%.
The diff coverage is 53.83%.

❗ Current head 7a33e9c differs from pull request most recent head 3b19e65. Consider uploading reports for the commit 3b19e65 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #499      +/-   ##
==========================================
+ Coverage   37.80%   39.39%   +1.58%     
==========================================
  Files         478      482       +4     
  Lines       68716    67501    -1215     
  Branches     8002     7872     -130     
==========================================
+ Hits        25981    26591     +610     
+ Misses      42647    40816    -1831     
- Partials       88       94       +6     
Impacted Files Coverage Δ
src/Acoustics.Shared/Csv/CsvSetPointConverter.cs 28.57% <ø> (ø)
.../Acoustics.Shared/Extensions/IntervalExtensions.cs 83.47% <ø> (ø)
...coustics.Shared/ImageSharp/ImageSharpExtensions.cs 66.66% <ø> (ø)
src/Acoustics.Tools/Audio/AbstractUtility.cs 54.40% <0.00%> (+0.76%) ⬆️
src/AnalysisBase/AbstractStrongAnalyser.cs 20.83% <ø> (ø)
src/AnalysisBase/Citation.cs 0.00% <0.00%> (ø)
src/AnalysisPrograms/AED.cs 30.76% <0.00%> (-0.16%) ⬇️
src/AnalysisPrograms/AcousticIndices.cs 60.00% <0.00%> (-0.21%) ⬇️
src/AnalysisPrograms/Audio2InputForConvCNN.cs 9.69% <0.00%> (-0.03%) ⬇️
src/AnalysisPrograms/ChannelIntegrity.cs 4.54% <0.00%> (-0.07%) ⬇️
... and 200 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 dd5e5e1...3b19e65. Read the comment docs.

@atruskie
Copy link
Member

Hi @towsey. Good tests but the issue was with the generic recogniser and not the multi recogniser.

You probably just need to add the array scaling call in the generic recogniser class.

But the way, this is what the imageWidth argument is used for (your comments frequently indicate you don't know what it is for). It's used to define a fixed width images should be when rendered. Not applicable here but I was reminded by the subject.

@towsey
Copy link
Contributor Author

towsey commented Jun 13, 2021

I have included a loop to rescale plots to fit the accompanying spectrogram.

var image = SpectrogramTools.GetSonogramPlusCharts(results.Sonogram, results.NewEvents, results.Plots, null);
image.SaveAsPng("C:/temp/image.png");
var image1 = SpectrogramTools.GetSonogramPlusCharts(results.Sonogram, results.NewEvents, results.Plots, null);
image1.Save(this.TestOutputDirectory + "\\image1.png");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You cannot use backslashes - our tests run on non-windows paltforms.

Use the this.TestOutputDirectory.CombineFile("WhistleSpectrogram.png")


// create the image for visual confirmation
var concatImage1 = ImageTools.CombineImagesVertically(imageList1);
concatImage1.Save(this.TestOutputDirectory + "ConcatImage1.png");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not join paths with the + operator.

Use the SaveTestOutput function

This comment applies to all usages in this class.

@towsey
Copy link
Contributor Author

towsey commented Jun 14, 2021

Very soryy but unfortunately, the work involved in fixing this aparnetly small Plot scaling problem got significantly more difficult because I used the OscillationEvent class in the unit test. It turned out that the functionality for much of Oscillation events was not working. So this latest commit is mostly about fixing oscillation events.

Copy link
Member

@atruskie atruskie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've suggested one change. When you're ready let me know and I'll merge this in

Comment on lines 1213 to 1233
public void TestOscillationPlusHarmonics()
{
var config = new GenericRecognizer.GenericRecognizerConfig()
{
Profiles = new Dictionary<string, object>()
{
{
"TestBlob", new BlobParameters()
{
FrameSize = 1024,
FrameStep = 1024,
MaxHertz = 7200,
MinHertz = 4800,
BgNoiseThreshold = 0.0,
BottomHertzBuffer = 1000,
TopHertzBuffer = 500,
DecibelThresholds = new double?[] { 3.0 },
}
},
{
"LowerBandDTMF_z",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the name of the test method and test profile do not match up.

DTMF tones are harmonic not oscillations.

Probably not wrong but it is confusing

@atruskie atruskie changed the title Issue469 rescaling of plot graphs Issue #469 rescaling of plot graphs Jun 16, 2021
Fixes #469

Update MultiRecognizer.cs

Issue #469 Minor changes. Except change the FFT window from Hamming to Hanning.

Update DataToolsTests.cs

Issue #469 Add a new test method to check that the scaling of image plots is working.

Update GenericRecognizerTests.cs

Issue #469 Remove hard-coded path.

Update GenericRecognizer.cs

Issue #469 Add in code to rescale the plots to fit the current spectrogram.

Changed the signature to SpectrogramTools.GetSonogramPlusCharts()

Issue #469 Changed the signature to SpectrogramTools.GetSonogramPlusCharts() so that it also includes the title of the spectrogram. THis affected these 6 methods.

Rescaling of the plots happens here.

Issue #469 This is ensures that all plots are the same length as the last spectrogram.

Update SpectrogramTools.cs

Issue #469  Fix the drawing of spectrograms.

Update BlobEvent.cs

Issue #469 The plot was being added twice for Blob events. Removed one of them.

Update OscillationEvent.cs

Issue #469
1: Added the Periodicity Property to the Oscillation Event class.
2: Fixed the Draw() method for Oscillation events. The events were not drawing.
3: Added a method to trim Oscillation events using the decibel values as a guide to where the true bounds should be.

Update Oscillations2019.cs

Issue #469 The main change is due to changing the signature of  the ConvertOscillationScores2Events() method

Update Oscillations2012.cs

Issue #469
1: Delete the GetOscillationFrequency() method
2: Change the way a new Oscillation event is initialised.

Update GenericRecognizerTests.cs

Issue #469 1: Write a new test method. Its original purpose was to satisfy Anthony's intention of having two profiles with different Windows. This was a way to get the plot scaling fixed. Unfortunately a lot of the functionality to do with the OscillationEvent class was not working. I used this test method to get it working.

Update OscillationEvent.cs

Issue #469 Rejig the calculation of the oscillation period to make it more accurate.

Adjust start and end time test results

Issue #469 1: Adjust event expected start and end times for test results involving oscillations. These adjustments brought about by tightening the bounds of an oscillation event.
2: Add periodicity result into the test. The periodicity is the property unique to oscillation events.

Update GenericRecognizerTests.cs

Issue #469 Change name of test method to match the profiles actually used.

Update GenericRecognizerTests.cs

Issue #469 Update name of the test method a third time! At present there is no method to convert type AcousticEvent directly to type BlobEvent. Only to type SpectralEvent.

Update GenericRecognizerTests.cs

Issue #469 Change name of profile to be consistent with Blob profile name.
@atruskie atruskie force-pushed the Issue469_Rescaling_of_Plot_Graphs branch from eb79861 to 7a65f8f Compare June 16, 2021 03:29
@atruskie atruskie merged commit e76cbc4 into master Jun 16, 2021
@atruskie atruskie deleted the Issue469_Rescaling_of_Plot_Graphs branch June 16, 2021 03:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plot graphs do not rescale when combined with plots from other spectrogram scales
2 participants