Issue492 revise mfcc code #497
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Issue #492 Minor changes to ensure that Delta and DoubleDelta features are being calculated when set true in the config file.
Issue #492 Add ing comments for clarity. Adding more mfcc parameters so user has more control over the calculations. Previously there were default values for some parameters.
Issue #492 Fixing up the calculation of mfcc feature vectors required the incorporation of signal pre-emphasis as an option. This is supposed to be good for speech recognition. All these changes are just related to adding in the preemphasis boolean to the method DSP_Frames.ExtractEnvelopeAndFfts().
Issue #492 This mfcc processing step was supposed to be helpful. However the ceptral features it produced were noisy, so did not incorporate but may be useful in some other context.
Issue #492
Issue #492 Add another constructor to MfccConfiguration class because the existing constructor uses discontinued dictionary.
Issue #492 Begin work on unit tests for some spectrogram classes.
Issue #492 Small changes to comment and method name
Issue #492 1) Correct an error in the calculation of delta and double-delta coefficients. 2) Remove duplication of a tricky method that normalizes spectral values for window power and SR. 3) Fix up method comments.
Issue #492 Add simple matrix method. Required after removing aforementioned method duplication.
Issue #492 Remove obsolete code.
Issue #492 Ensure that the mel scale spectrogram parameters are passed to the drawing method.
Issue #492 Make method accept double
Issue #492 Main change is to ensure that the epsilon value passed to GetLogEnergySpectrogram() is squared before being used.
Issue #492 Main change is to remove spectrogram normalisation at line 79. So the Make method returns a spectrogram having decibel values.
Issue #492 Change old line 88. When preparing a cepstrogram, the linear filter bank is not a valid option. Other changes are to comments.
Issue #492 Once all three tests were working as expected, had to rewrite three binary files in Test/Fixtures.
Codecov Report
@@ Coverage Diff @@
## master #497 +/- ##
==========================================
+ Coverage 38.53% 38.72% +0.18%
==========================================
Files 482 482
Lines 67647 67466 -181
Branches 7901 7870 -31
==========================================
+ Hits 26065 26123 +58
+ Misses 41491 41252 -239
Partials 91 91
Continue to review full report at Codecov.
|
|
@towsey please confirm all tests pass manually (CI still failing). Either run all tests from the test explorer, or run |
atruskie
requested changes
Jun 7, 2021
There was a problem hiding this comment.
A good PR.
Only changes required are with the unit tests.
- The best solution for tests is encode known values into the test files.
- Another good solution is to encode tolerable range for a result based on a hand calculated example
- The bin files you are using aren't even that useful since you only compare aggregate values anyway - maybe you don't need to commit the spectrums at all?
- If you do need to commit them, maybe a CSV is a better choice? At least the data would not be opaque
- The best choice would be arrow or protobuf formats, but we don't support that
- BinarySerializer is deprecated and will be removed in furture versions of .NET - we can't keep adding new code that depends on it
Also, please ensure you use OutPutDirectoryTest as the base class for writing files during tests. In particular the SaveTestOutput function will place files in the daily folder we made for you to easily find results:
| @@ -12,13 +12,39 @@ public class SpectrogramGeneratorConfig : AnalyzerConfig | |||
| #pragma warning disable SA1623 // Property summary documentation should match accessors | |||
| public int WaveformHeight { get; set; } = 100; | |||
|
|
|||
| public double BgNoiseThreshold { get; set; } = 3.0; | |||
| public double BgNoiseThreshold { get; set; } = 0.0; | |||
There was a problem hiding this comment.
Do you intend for this default to be global?
Is it changed in the config file as well?
Comment on lines
+22
to
+47
| /// <summary> | ||
| /// CEPSTROGRAM - PARAMETER. | ||
| /// Do pre-emphasis prior to FFT. | ||
| /// </summary> | ||
| public bool DoPreemphasis { get; set; } = false; | ||
|
|
||
| /// <summary> | ||
| /// CEPSTROGRAM - PARAMETER | ||
| /// The size of the Mel-scale filter bank. | ||
| /// The default value is 64. | ||
| /// THe minimum I have seen referenced = 26. | ||
| /// </summary> | ||
| public int FilterbankCount { get; set; } = 64; | ||
|
|
||
| /// <summary> | ||
| /// CEPSTROGRAM - PARAMETER. | ||
| /// Include the delta features in the returned MFCC feature vector. | ||
| /// </summary> | ||
| public bool IncludeDelta { get; set; } = false; | ||
|
|
||
| /// <summary> | ||
| /// CEPSTROGRAM - PARAMETER. | ||
| /// Include the delta-delta or acceleration features in the returned MFCC feature vector. | ||
| /// </summary> | ||
| public bool IncludeDoubleDelta { get; set; } = false; | ||
|
|
There was a problem hiding this comment.
Ensure equivalent parameters / docs added to the default config file
| /// see https://docs.microsoft.com/en-us/dotnet/standard/base-types/custom-numeric-format-strings. | ||
| /// </summary> | ||
| /// <param name="array">an array of double.</param> | ||
| /// <param name="format">format string.</param> | ||
| public static string WriteArrayAsCsvLine(double[] array, string format) |
There was a problem hiding this comment.
I'd prefer to see methods in this file deleted, but documenting is better than nothing
No changes requested however.
| /// where number of zeros after the point is the number of decimal places. | ||
| /// For more detail on custom numeric format string, | ||
| /// see https://docs.microsoft.com/en-us/dotnet/standard/base-types/custom-numeric-format-strings. | ||
| /// </summary> |
There was a problem hiding this comment.
Again, prefer deletion over documentation. No changes requested however.
| [TestCategory("Spectrograms")] | ||
| public class MelSpectrogramTest | ||
| { | ||
| public const double Delta = 0.000_000_001; |
There was a problem hiding this comment.
we have allowable delta elsewhere?
not useful? Leave a comment for this value please
Comment on lines
92
to
94
| // check that the image is what you expect. | ||
| //var image = melSpectrogram.GetImage(); | ||
| //image.Save("C:\\temp\\melScaleimage_preemphasisTEST.png"); |
Comment on lines
96
to
101
| var sumFile = PathHelper.ResolveAsset("SpectrogramTestResults", "BAC2_20071008-085040_MelSpectrogramDataColumnSums_WithPreemphasis.bin"); | ||
|
|
||
| // uncomment this to update the binary data. Should be rarely needed | ||
| //Binary.Serialize(sumFile, columnSums); | ||
|
|
||
| var expectedColSums = Binary.Deserialize<double[]>(sumFile); |
There was a problem hiding this comment.
avoid binary serializer - it is very brittle
Comment on lines
160
to
169
| // check that the image is something like you expect. | ||
| //var image = cepstrogram.GetImage(); | ||
| //image.Save("C:\\temp\\mfccimage_WithPreemphasisTEST.png"); | ||
|
|
||
| var sumFile = PathHelper.ResolveAsset("SpectrogramTestResults", "BAC2_20071008-085040_CeptrogramDataColumnSums_WithPreemphasis.bin"); | ||
|
|
||
| // uncomment this to update the binary data. Should be rarely needed | ||
| //Binary.Serialize(sumFile, columnSums); | ||
|
|
||
| var expectedColSums = Binary.Deserialize<double[]>(sumFile); |
Comment on lines
244
to
267
| //var image1 = melSpectrogram.GetImage(); | ||
| //image1.Save("C:\\temp\\mfccimage_no-preemphasisTEST1.png"); | ||
|
|
||
| // Test sonogram data matrix by comparing the vector of column sums. | ||
| double[] columnSums = MatrixTools.SumColumns(cepM); | ||
|
|
||
| var sumFile = PathHelper.ResolveAsset("SpectrogramTestResults", "BAC2_20071008-085040_CeptrogramDataColumnSums_WithoutPreemphasis.bin"); | ||
|
|
||
| // uncomment this to update the binary data. Should be rarely needed | ||
| //Binary.Serialize(sumFile, columnSums); | ||
|
|
||
| var expectedColSums = Binary.Deserialize<double[]>(sumFile); | ||
| var totalDelta = expectedColSums.Zip(columnSums, ValueTuple.Create).Select(x => Math.Abs(x.Item1 - x.Item2)).Sum(); | ||
| var avgDelta = expectedColSums.Zip(columnSums, ValueTuple.Create).Select(x => Math.Abs(x.Item1 - x.Item2)).Average(); | ||
| Assert.AreEqual(expectedColSums[0], columnSums[0], Delta, $"\nE: {expectedColSums[0]:R}\nA: {columnSums[0]:R}\nD: {expectedColSums[0] - columnSums[0]:R}\nT: {totalDelta:R}\nA: {avgDelta}\nn: {expectedColSums.Length}"); | ||
| CollectionAssert.That.AreEqual(expectedColSums, columnSums, Delta); | ||
|
|
||
| // Do the test again but going directly to a cepstrogram. | ||
| var cepstrogram = new SpectrogramCepstral(config, this.recording.WavReader); | ||
| var data = cepstrogram.Data; | ||
|
|
||
| // check that the image is something like you expect. | ||
| //var image = cepstrogram.GetImage(); | ||
| //image.Save("C:\\temp\\mfccimage_no-preemphasisTEST2.png"); |
There was a problem hiding this comment.
again,
remove commented file writes (replace with actual file writes via SaveTestOutput)
don't use binary serializer
| // Test sonogram data matrix by comparing the vector of column sums. | ||
| double[] columnSums = MatrixTools.SumColumns(amplSpectrogram); | ||
|
|
||
| var sumFile = PathHelper.ResolveAsset("EnvelopeAndFft", "BAC2_20071008-085040_DataColumnSums.bin"); |
Issue #792 Required an extra binary file to cover a previously added test involving pre-emphasis
Issue $492 Refactor the test class to accord with Anthony's requests.
Issue #492 remove use of binary files for testing matrix values. Also use the proper system for checking test images.
Issue #492 These are no longer required for testing. Now used precalculated values.
Issue #492 Bring SpectrogramGenerator config file up to date with previous changes.
Issue #492 Bring test files up to date with new requirements. No longer use pre-calculated binary files. Instead usea limited number of pre-calculated values.
|
I believe I have dealt with these issues now. I hope so because I have just deleted a bunch of binary fixtures! |
Issue #492 THis error was due to different config values between config file and default constructor.
atruskie
approved these changes
Jun 9, 2021
Issue #492 Removed unnecessary references to output directory as requested by Anthony.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Revision of MFCC code
The purpose of this isse was to check and rework the mfcc code where necessary.
Parts of this code had not been used or revised in a decade.
Closes #492
Changes
Changes were made in light of my rereading some literature on the calculation of mfcc features. In particular the delta and double-delta features were not being calculated correctly.
I added three unit tests for the generation of melscale spectrograms and cepstrograms.
Issues
I am not aware of any remaining issues that need to be addressed re mfcc calculations.
Visual Changes
The appearance of the mel scale and mfcc spectrograms has not changed noticeably.
Final Checklist