Improve Create4Sonograms

atruskie commented 6 years ago

Expected behaviour

The UX of Create4Sonograms class (The DrawSpectrograms command) should be on par with our other commands.

Actual behaviour

A number of sub-par problems:

The CLI has a non-standard output option (should be an argument)
The config file required is not specified in the documentation
The documentation for the config file being automatically resolved is incorrect
The code uses old-style untyped configuration
No matter which output argument you give it will error about the file not existing
- Bug for this in these lines: https://github.com/QutEcoacoustics/audio-analysis/blob/6ff412d4d83319a7c8755ef1b896bda00f1e2ed3/src/AnalysisPrograms/Production/Validation/ExistingFileAttribute.cs#L52-L56
There are no unit tests for this code
We really should make Create4Sonograms implment IAnalyzer2
It seems the functionality is duplicated with Audio2Sonogram - they both draw 4 spectrograms!

Steps to reproduce the behaviour

What command did you use?
- C:\Temp> AnalysisPrograms.exe drawspectrograms .\min2.wav "C:\Work\Github\audio-analysis\src\AnalysisConfigFiles\Towsey.Sonogram.yml" --output spectrogram.png
What config file did you use?
- The standad Towsey.Sonogram.yml
Where can we find the source data used?
- Any short wav file

Any other details

Deficiencies brought to our attention by Mangalam (JCU)

towsey commented 5 years ago

My suggestion is that we deprecate Create4Sonograms and put all functionality we want inot Audio2Csv. I last worked on Audio2Csv in 2017 and looks to be OK now. The method Audio2Sonogram.TESTMETHOD_DrawFourSpectrograms() still produces expected output.

atruskie commented 5 years ago

Your message is a little unclear.

audio2csv is a generic analysis runner - we shouldn't put specific analysis code in it.

If you meant generating spectrograms in the Towsey.Acoustic index generation analysis, then no again. Apart from that analysis doing too much already (and thus being overly complex), the arity of the output doesn't match; for indices, we get one file of indices per recording, for Create4Sonograms outputs an image for every segment.

The correct course of action is to reimplement it as an IAnalyzer, which I've already started doing in 0fc8400.

You may have a point on removing the entry point for Create2Sonograms but then again, it provides a nice amount of discoverability and simplicity.

towsey commented 5 years ago

I had assumed that AnalysisPrograms would retain one activity (audio2sonogram) which produces one or more sonogram images given an input recording of suitable length. The "test" method I wrote last year, TESTMETHOD_DrawFourSpectrograms(), still works and still produces an image that combines several sonograms of a one-minute recording. However we get there, the functionality is worth keeping. I will cease working on this until it is clearer how to proceed.

atruskie commented 5 years ago

Yes, I'd agree with you stopping work on this. This issue wasn't assigned, and I've previously done work for it.

towsey commented 5 years ago

Nevertheless there is some useful functionality that I owuld not like to lose and is appropriate for exploring a sonogam. In general, it seems to me that you need to set up the outer structure for some functionality before I actually code the function. Perhaps we can sort that out tomorrow. I think I should be able to work on Issue #186 because in the first instance it just involves finding the source of the problem.

atruskie commented 5 years ago

Note: partial work done in https://github.com/QutEcoacoustics/audio-analysis/pull/227/commits/e557cf1548c8c198f02ae14691bf85415a9ab85d

QutEcoacoustics / audio-analysis