APM Quality Assessment, simulation controller and libraries

Simulation controller (shell arguments parser and simulation runner) and libraries (data access, noise generators, evaluation scores).
Execution flag added to the .py and .sh scripts.
BUILD.gn files adapted (see :lib), APM config files moved.

BUG=webrtc:7218
NOTRY=True

Review-Url: https://codereview.webrtc.org/2715943002
Cr-Commit-Position: refs/heads/master@{#17007}
Committed: https://chromium.googlesource.com/external/webrtc/+/a4b1d31207a9d73ec9c85a443375d1ba575fea55

[AleBzk, 2017-02-24 12:46:43]

Description was changed from

==========
Simulation controller (shell arguments parser and simulation runner) and
libraries (data access, noise generators, evaluation scores).
Execution flag added to the .py and .sh scripts.
BUILD.gn files adapted (see :lib), APM config files moved.

BUG=webrtc:7218
==========

to

==========
Simulation controller (shell arguments parser and simulation runner) and
libraries (data access, noise generators, evaluation scores).
Execution flag added to the .py and .sh scripts.
BUILD.gn files adapted (see :lib), APM config files moved.

BUG=webrtc:7218
==========

[AleBzk, 2017-02-24 12:46:44]

alessiob@webrtc.org changed reviewers:
+ aleloi@webrtc.org, ivoc@webrtc.org

[AleBzk, 2017-02-24 12:52:42]

Simulation controller (shell arguments parser and simulation runner) and
libraries (data access, noise generators, evaluation scores).

Everything starts with the apm_quality_assessment.sh bash script which calls
apm_quality_assessment.py (the new entry).
The latter parses the command line arguments and runs the simulator, which
iterates over a number of parameter sets (e.g., noise generators, APM
configurations).

The list of noise generators and evaluation scores is generated by a decorator
that register each class.
This is used to know the command line argument values for the corresponding
options.

Note that I added TODO comments for the part that are not implemented.

Ignore presubmit message with type "missing-super-argument", they are raised
only because I'm using Python 3 (while the linter is tuned for Python 2).

[ivoc, 2017-02-24 14:41:48]

Very nice! I don't write/review python code very often, so there were some
points that look a bit strange to me (or at least different from our C++
conventions), but might be okay in python, such as:
- Multiple classes in one file
- Filenames with a '-' in them
Also, I hope to see some unit tests in one of the next CLs :-)

https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
File
webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.py
(right):

https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.py:1:
#!/usr/bin/env python3
Why is this is some python files but not in some others?

https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.py:43:
'arguments with which the auiodproc_f tool is '
audioproc_f

https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores.py
(right):

https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores.py:12:

Remove empty line please.

https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores.py:21:
Decorator with which automatically register the classes that extend
I would rephrase as: Decorator to automatically ...

https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py
(right):

https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:12:

Remove empty line, please.

https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:21:
Decorator with which automatically register the classes that extend
Decorator to...

[hlundin-webrtc, 2017-02-24 16:36:31]

On 2017/02/24 14:41:48, ivoc wrote:
> Very nice! I don't write/review python code very often, so there were some
> points that look a bit strange to me (or at least different from our C++
> conventions), but might be okay in python, such as:
> - Multiple classes in one file
> - Filenames with a '-' in them
> Also, I hope to see some unit tests in one of the next CLs :-)
> 
>
https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
> File
>
webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.py
> (right):
> 
>
https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
>
webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.py:1:
> #!/usr/bin/env python3
> Why is this is some python files but not in some others?
> 
>
https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
>
webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.py:43:
> 'arguments with which the auiodproc_f tool is '
> audioproc_f
> 
>
https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
> File
>
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores.py
> (right):
> 
>
https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
>
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores.py:12:
> 
> Remove empty line please.
> 
>
https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
>
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores.py:21:
> Decorator with which automatically register the classes that extend
> I would rephrase as: Decorator to automatically ...
> 
>
https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
> File
>
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py
> (right):
> 
>
https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
>
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:12:
> 
> Remove empty line, please.
> 
>
https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
>
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:21:
> Decorator with which automatically register the classes that extend
> Decorator to...

I don't know if we usually follow this, but here is a Google style guide for
python: https://google.github.io/styleguide/pyguide.html.

[AleBzk, 2017-02-27 14:23:47]

Thanks a lot for your comments.

@Ivo: yes, I should definitely start writing unit tests, at least for those
parts that can easily break :)
As for having multiple classes in a single file, consider that in Python files
(not only directories) are seen as modules.
In fact the linter never complained.

[AleBzk, 2017-03-01 09:07:12]

https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
File
webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.py
(right):

https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.py:1:
#!/usr/bin/env python3
On 2017/02/24 14:41:47, ivoc wrote:
> Why is this is some python files but not in some others?

Thanks for asking. As documented in our style guide
(https://google.github.io/styleguide/pyguide.html?showone=Shebang_Line#Shebang...),
it's only needed for those script that run from shell. That's why you don't find
that line in the quality_assessment package.

https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.py:43:
'arguments with which the auiodproc_f tool is '
On 2017/02/24 14:41:47, ivoc wrote:
> audioproc_f

Acknowledged.

https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores.py
(right):

https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores.py:12:

On 2017/02/24 14:41:47, ivoc wrote:
> Remove empty line please.

Acknowledged.

https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores.py:21:
Decorator with which automatically register the classes that extend
On 2017/02/24 14:41:47, ivoc wrote:
> I would rephrase as: Decorator to automatically ...

Acknowledged.

https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py
(right):

https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:12:

On 2017/02/24 14:41:47, ivoc wrote:
> Remove empty line, please.

Acknowledged.

https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:21:
Decorator with which automatically register the classes that extend
On 2017/02/24 14:41:48, ivoc wrote:
> Decorator to...

Acknowledged.

[AleBzk, 2017-03-01 10:00:57]

Patchset #4 (id:60001) has been deleted

[AleBzk, 2017-03-01 12:03:41]

@Ivo: I added initial unit test files (see the new files added in Patch 5, i.e.,
diff 4)

[ivoc, 2017-03-01 22:51:29]

Nice work, lgtm. You will need to get a lgtm from one of the audio_processing
owners before you can land this.

https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
File
webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.py
(right):

https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.py:1:
#!/usr/bin/env python3
On 2017/03/01 09:07:12, AleBzk wrote:
> On 2017/02/24 14:41:47, ivoc wrote:
> > Why is this is some python files but not in some others?
> 
> Thanks for asking. As documented in our style guide
>
(https://google.github.io/styleguide/pyguide.html?showone=Shebang_Line#Shebang...),
> it's only needed for those script that run from shell. That's why you don't
find
> that line in the quality_assessment package.

Acknowledged.

[aleloi2, 2017-03-01 23:48:09]

On 2017/03/01 22:51:29, ivoc wrote:
> Nice work, lgtm. You will need to get a lgtm from one of the audio_processing
> owners before you can land this.
> 
>
https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
> File
>
webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.py
> (right):
> 
>
https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
>
webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.py:1:
> #!/usr/bin/env python3
> On 2017/03/01 09:07:12, AleBzk wrote:
> > On 2017/02/24 14:41:47, ivoc wrote:
> > > Why is this is some python files but not in some others?
> > 
> > Thanks for asking. As documented in our style guide
> >
>
(https://google.github.io/styleguide/pyguide.html?showone=Shebang_Line#Shebang...),
> > it's only needed for those script that run from shell. That's why you don't
> find
> > that line in the quality_assessment package.
> 
> Acknowledged.

The tests can be added to PRESUBMIT.py so that they are run every time something
changes. Here's how to do that:
https://codereview.webrtc.org/1999113002/diff/460001/PRESUBMIT.py

[aleloi2, 2017-03-02 00:25:58]

On 2017/03/01 23:48:09, aleloi2 wrote:
> On 2017/03/01 22:51:29, ivoc wrote:
> > Nice work, lgtm. You will need to get a lgtm from one of the
audio_processing
> > owners before you can land this.
> > 
> >
>
https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
> > File
> >
>
webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.py
> > (right):
> > 
> >
>
https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
> >
>
webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.py:1:
> > #!/usr/bin/env python3
> > On 2017/03/01 09:07:12, AleBzk wrote:
> > > On 2017/02/24 14:41:47, ivoc wrote:
> > > > Why is this is some python files but not in some others?
> > > 
> > > Thanks for asking. As documented in our style guide
> > >
> >
>
(https://google.github.io/styleguide/pyguide.html?showone=Shebang_Line#Shebang...),
> > > it's only needed for those script that run from shell. That's why you
don't
> > find
> > > that line in the quality_assessment package.
> > 
> > Acknowledged.
> 
> The tests can be added to PRESUBMIT.py so that they are run every time
something
> changes. Here's how to do that:
> https://codereview.webrtc.org/1999113002/diff/460001/PRESUBMIT.py

Can you rebase this on top of https://codereview.webrtc.org/2711923002/? It must
be done in order to land this CL anyway, and it's easier to see what has changed
this way.

[ivoc, 2017-03-02 00:36:48]

On 2017/03/02 00:25:58, aleloi2 wrote:
> On 2017/03/01 23:48:09, aleloi2 wrote:
> > On 2017/03/01 22:51:29, ivoc wrote:
> > > Nice work, lgtm. You will need to get a lgtm from one of the
> audio_processing
> > > owners before you can land this.
> > > 
> > >
> >
>
https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
> > > File
> > >
> >
>
webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.py
> > > (right):
> > > 
> > >
> >
>
https://codereview.webrtc.org/2715943002/diff/1/webrtc/modules/audio_processi...
> > >
> >
>
webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.py:1:
> > > #!/usr/bin/env python3
> > > On 2017/03/01 09:07:12, AleBzk wrote:
> > > > On 2017/02/24 14:41:47, ivoc wrote:
> > > > > Why is this is some python files but not in some others?
> > > > 
> > > > Thanks for asking. As documented in our style guide
> > > >
> > >
> >
>
(https://google.github.io/styleguide/pyguide.html?showone=Shebang_Line#Shebang...),
> > > > it's only needed for those script that run from shell. That's why you
> don't
> > > find
> > > > that line in the quality_assessment package.
> > > 
> > > Acknowledged.
> > 
> > The tests can be added to PRESUBMIT.py so that they are run every time
> something
> > changes. Here's how to do that:
> > https://codereview.webrtc.org/1999113002/diff/460001/PRESUBMIT.py
> 
> Can you rebase this on top of https://codereview.webrtc.org/2711923002/ It
must
> be done in order to land this CL anyway, and it's easier to see what has
changed
> this way.

One extra comment that applies to this and your other CLs as well, please add a
one-line summary at the top of your CL description, with a blank line to
separate it from the summary.

[aleloi2, 2017-03-02 01:14:02]

aleloi@google.com changed reviewers:
+ aleloi@google.com

[aleloi2, 2017-03-02 01:14:03]

Nice patch!

LGTM, but I have some suggestions inline.

Also, is it possible to make the CL description clearer? E.g. mention apm
quality assessment in the title and description.

https://codereview.webrtc.org/2715943002/diff/100001/webrtc/modules/audio_pro...
File
webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.py
(right):

https://codereview.webrtc.org/2715943002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.py:70:
logging.basicConfig(level=logging.DEBUG)
Interesting! I didn't know about the logging system.

https://codereview.webrtc.org/2715943002/diff/100001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/test/py_quality_assessment/output/README.md
(right):

https://codereview.webrtc.org/2715943002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/output/README.md:1:
You can use this folder for the output generated by the apm_quality_assessment
scripts.
Perhaps this should be made the default output folder then? In the latest
version, argparse has a required argument for the output folder.

https://codereview.webrtc.org/2715943002/diff/100001/webrtc/modules/audio_pro...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/data_access.py
(right):

https://codereview.webrtc.org/2715943002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/data_access.py:13:
Makes a directory (recursively) and hides OSError exceptions.
Why should OSError be hidden? I can't understand this from the usage in
simulation.py

https://codereview.webrtc.org/2715943002/diff/100001/webrtc/modules/audio_pro...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores.py
(right):

https://codereview.webrtc.org/2715943002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores.py:45:
Compute the POLQA score. It requires that the POLQA_PATH environment variable
Can it be checked at construction that POLQUA_PATH is defined and perhaps that
it's a valid path? Following a guideline that errors should be detected as early
as possible. It's just a suggestion, doesn't have to be like that.

https://codereview.webrtc.org/2715943002/diff/100001/webrtc/modules/audio_pro...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py
(right):

https://codereview.webrtc.org/2715943002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:22:
# TODO(alessio): instance when implementation is ready.
Sorry, I don't understand this comment. Does it mean to fix the '=None' below?
They are initialized in '.run()', right?

https://codereview.webrtc.org/2715943002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:35:
Initializes paths and required instances before running all the simulations.
Perhaps add 'Then runs the simulations.'

https://codereview.webrtc.org/2715943002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:41:
self._NOISE_GENERATOR_CLASSES[name]() for name in noise_generator_names]
A key error happens if a user passes a wrong name, right? Is that the intention?

https://codereview.webrtc.org/2715943002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:72:

I think we can consider using itertools.product here to have less layers of
nesting. Less nesting makes it simpler to read and easier to spot bugs.

[AleBzk, 2017-03-02 09:58:38]

Hi, thanks to both of you for your comments.
Everything has been addressed. You can see my replies to each comment below.

https://codereview.webrtc.org/2715943002/diff/100001/webrtc/modules/audio_pro...
File
webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.py
(right):

https://codereview.webrtc.org/2715943002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.py:70:
logging.basicConfig(level=logging.DEBUG)
On 2017/03/02 01:14:03, aleloi2 wrote:
> Interesting! I didn't know about the logging system.

Done.

https://codereview.webrtc.org/2715943002/diff/100001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/test/py_quality_assessment/output/README.md
(right):

https://codereview.webrtc.org/2715943002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/output/README.md:1:
You can use this folder for the output generated by the apm_quality_assessment
scripts.
On 2017/03/02 01:14:03, aleloi2 wrote:
> Perhaps this should be made the default output folder then? In the latest
> version, argparse has a required argument for the output folder.

Nice idea! Thanks!

https://codereview.webrtc.org/2715943002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/output/README.md:1:
You can use this folder for the output generated by the apm_quality_assessment
scripts.
On 2017/03/02 01:14:03, aleloi2 wrote:
> Perhaps this should be made the default output folder then? In the latest
> version, argparse has a required argument for the output folder.

Done.

https://codereview.webrtc.org/2715943002/diff/100001/webrtc/modules/audio_pro...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/data_access.py
(right):

https://codereview.webrtc.org/2715943002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/data_access.py:13:
Makes a directory (recursively) and hides OSError exceptions.
On 2017/03/02 01:14:03, aleloi2 wrote:
> Why should OSError be hidden? I can't understand this from the usage in
> simulation.py

Premise: in my tool I implicitly implemented checkpoints and cache, so a run may
reuse data from a previous one.
Answer: the directory might already exist, just wanted to avoid code duplication
every time I add directories.

To improve readability and avoid issues with missing write permissions, I
explicitly check if the directory already exists and just return if so.

Thanks for this comment!

https://codereview.webrtc.org/2715943002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/data_access.py:13:
Makes a directory (recursively) and hides OSError exceptions.
On 2017/03/02 01:14:03, aleloi2 wrote:
> Why should OSError be hidden? I can't understand this from the usage in
> simulation.py

Done.

https://codereview.webrtc.org/2715943002/diff/100001/webrtc/modules/audio_pro...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores.py
(right):

https://codereview.webrtc.org/2715943002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores.py:45:
Compute the POLQA score. It requires that the POLQA_PATH environment variable
On 2017/03/02 01:14:03, aleloi2 wrote:
> Can it be checked at construction that POLQUA_PATH is defined and perhaps that
> it's a valid path? Following a guideline that errors should be detected as
early
> as possible. It's just a suggestion, doesn't have to be like that.

Reminding best practices is always appreciated. Thanks!
The code is not there yet, I have another CL which implements the POLQA score.
I will do the check you suggest in there.

https://codereview.webrtc.org/2715943002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores.py:45:
Compute the POLQA score. It requires that the POLQA_PATH environment variable
On 2017/03/02 01:14:03, aleloi2 wrote:
> Can it be checked at construction that POLQUA_PATH is defined and perhaps that
> it's a valid path? Following a guideline that errors should be detected as
early
> as possible. It's just a suggestion, doesn't have to be like that.

Acknowledged.

https://codereview.webrtc.org/2715943002/diff/100001/webrtc/modules/audio_pro...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py
(right):

https://codereview.webrtc.org/2715943002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:22:
# TODO(alessio): instance when implementation is ready.
On 2017/03/02 01:14:03, aleloi2 wrote:
> Sorry, I don't understand this comment. Does it mean to fix the '=None' below?
> They are initialized in '.run()', right?

Sorry for not being clear here.
This CL still misses two classes that will come in separate CLs.
The two members should be initialized in the constructor, but for now I set them
to None.
If you check, they're not used (yet).

I won't change the comment to keep it compact and because it is removed in
another CL.

https://codereview.webrtc.org/2715943002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:22:
# TODO(alessio): instance when implementation is ready.
On 2017/03/02 01:14:03, aleloi2 wrote:
> Sorry, I don't understand this comment. Does it mean to fix the '=None' below?
> They are initialized in '.run()', right?

Acknowledged.

https://codereview.webrtc.org/2715943002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:35:
Initializes paths and required instances before running all the simulations.
On 2017/03/02 01:14:03, aleloi2 wrote:
> Perhaps add 'Then runs the simulations.'

Done.

https://codereview.webrtc.org/2715943002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:41:
self._NOISE_GENERATOR_CLASSES[name]() for name in noise_generator_names]
On 2017/03/02 01:14:03, aleloi2 wrote:
> A key error happens if a user passes a wrong name, right? Is that the
intention?

Yes and no :)

The client code should handle the exception.
But if you look in apm_quality_assessment.py there is not try/except.
That's because argparse is already taking care of accepting only valid input.

If there is any best practice I should follow (e.g., handling the KeyError
exception anyways), just let me know and I'll patch in a separate CL.

https://codereview.webrtc.org/2715943002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:41:
self._NOISE_GENERATOR_CLASSES[name]() for name in noise_generator_names]
On 2017/03/02 01:14:03, aleloi2 wrote:
> A key error happens if a user passes a wrong name, right? Is that the
intention?

Acknowledged.

https://codereview.webrtc.org/2715943002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:72:

On 2017/03/02 01:14:03, aleloi2 wrote:
> I think we can consider using itertools.product here to have less layers of
> nesting. Less nesting makes it simpler to read and easier to spot bugs.

True. I usually go for that option, I hate 45 degrees coding.
I took note of this and will write a separate CL to rewrite all the nested
loops.
Thanks!

https://codereview.webrtc.org/2715943002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:72:

On 2017/03/02 01:14:03, aleloi2 wrote:
> I think we can consider using itertools.product here to have less layers of
> nesting. Less nesting makes it simpler to read and easier to spot bugs.

Acknowledged.

[AleBzk, 2017-03-02 10:00:20]

Description was changed from

==========
Simulation controller (shell arguments parser and simulation runner) and
libraries (data access, noise generators, evaluation scores).
Execution flag added to the .py and .sh scripts.
BUILD.gn files adapted (see :lib), APM config files moved.

BUG=webrtc:7218
==========

to

==========
Simulation controller (shell arguments parser and simulation runner) and
libraries (data access, noise generators, evaluation scores).
Execution flag added to the .py and .sh scripts.
BUILD.gn files adapted (see :lib), APM config files moved.

BUG=webrtc:7218
NOTRY=True
==========

[AleBzk, 2017-03-02 13:44:38]

The CQ bit was checked by alessiob@webrtc.org

[AleBzk, 2017-03-02 13:44:39]

The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org,
aleloi@google.com
Link to the patchset: https://codereview.webrtc.org/2715943002/#ps120001 (title:
"comments from Alex addressed")

[commit-bot: I haz the power, 2017-03-02 13:44:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-02 13:47:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-02 13:47:56]

Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/14371)

[AleBzk, 2017-03-02 14:35:03]

The CQ bit was checked by alessiob@webrtc.org

[AleBzk, 2017-03-02 14:35:04]

The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org,
aleloi@google.com
Link to the patchset: https://codereview.webrtc.org/2715943002/#ps140001 (title:
"rebase")

[commit-bot: I haz the power, 2017-03-02 14:35:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-02 14:41:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-02 14:41:04]

Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/14376)

[aleloi2, 2017-03-02 16:54:11]

On 2017/03/02 14:41:04, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   presubmit on master.tryserver.webrtc (JOB_FAILED,
> http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/14376)

Ship it :)

[AleBzk, 2017-03-02 16:54:16]

alessiob@webrtc.org changed reviewers:
- aleloi@webrtc.org

[AleBzk, 2017-03-03 14:46:45]

The CQ bit was checked by alessiob@webrtc.org

[AleBzk, 2017-03-03 14:46:46]

The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org,
aleloi@google.com
Link to the patchset: https://codereview.webrtc.org/2715943002/#ps160001 (title:
"revert to Python 2")

[commit-bot: I haz the power, 2017-03-03 14:46:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-03 14:48:49]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1488552405527580,
"parent_rev": "4336d73bd3c8df53ec396e9600463c8e162e20b1", "commit_rev":
"a4b1d31207a9d73ec9c85a443375d1ba575fea55"}

[commit-bot: I haz the power, 2017-03-03 14:48:51]

Description was changed from

==========
Simulation controller (shell arguments parser and simulation runner) and
libraries (data access, noise generators, evaluation scores).
Execution flag added to the .py and .sh scripts.
BUILD.gn files adapted (see :lib), APM config files moved.

BUG=webrtc:7218
NOTRY=True
==========

to

==========
Simulation controller (shell arguments parser and simulation runner) and
libraries (data access, noise generators, evaluation scores).
Execution flag added to the .py and .sh scripts.
BUILD.gn files adapted (see :lib), APM config files moved.

BUG=webrtc:7218
NOTRY=True

Review-Url: https://codereview.webrtc.org/2715943002
Cr-Commit-Position: refs/heads/master@{#17007}
Committed:
https://chromium.googlesource.com/external/webrtc/+/a4b1d31207a9d73ec9c85a443...
==========

[commit-bot: I haz the power, 2017-03-03 14:48:53]

Committed patchset #8 (id:160001) as
https://chromium.googlesource.com/external/webrtc/+/a4b1d31207a9d73ec9c85a443...