POLQA evaluation score.

POLQA evaluation score.

BUG=webrtc:7218
NOTRY=True

Review-Url: https://codereview.webrtc.org/2718063003
Cr-Commit-Position: refs/heads/master@{#17509}
Committed: https://chromium.googlesource.com/external/webrtc/+/0deb594b2567808f4185243a57afd830bd201bac

[AleBzk, 2017-02-28 16:03:43]

Description was changed from

==========
POLQA evaluation score.

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

to

==========
POLQA evaluation score.

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

[AleBzk, 2017-02-28 16:03:43]

alessiob@webrtc.org changed reviewers:
+ kjellander@webrtc.org

[AleBzk, 2017-02-28 16:20:09]

Yet another CL on evaluation scores for the APM module. This is a wrapper to
POLQA.

[kjellander_webrtc, 2017-03-03 05:08:07]

https://codereview.webrtc.org/2718063003/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/2718063003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores.py:31:
def register_class(cls, class_to_register):
Chromium deviates from the standard Python style guide in two cases:
* Indentation: use 2, not 4, spaces
* Naming: Functions and Method names use CapWords() style, not
lower_with_under()
  The only exception to this rule is the main() function -- we do not use Main()
http://dev.chromium.org/chromium-os/python-style-guidelines

Please update te method names to match this in a separate CL (not this one).

https://codereview.webrtc.org/2718063003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores.py:126:
_POLQA_PATH = os.environ['POLQA_PATH']
Please don't use environment variables. Pass this information into the class as
an argument instead.

https://codereview.webrtc.org/2718063003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores.py:168:
assert len(data) == 2  # Two rows expected (header and values).
Please add a friendly message to be printed if the assert fails.

[AleBzk, 2017-03-03 12:37:56]

Hi Henrik,

Thanks so much for your comments.
Everything has been addressed.

https://codereview.webrtc.org/2718063003/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/2718063003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores.py:31:
def register_class(cls, class_to_register):
On 2017/03/03 05:08:07, kjellander_webrtc wrote:
> Chromium deviates from the standard Python style guide in two cases:
> * Indentation: use 2, not 4, spaces
> * Naming: Functions and Method names use CapWords() style, not
> lower_with_under()
>   The only exception to this rule is the main() function -- we do not use
Main()
> http://dev.chromium.org/chromium-os/python-style-guidelines
> 
> Please update te method names to match this in a separate CL (not this one).

Acknowledged.

https://codereview.webrtc.org/2718063003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores.py:126:
_POLQA_PATH = os.environ['POLQA_PATH']
On 2017/03/03 05:08:07, kjellander_webrtc wrote:
> Please don't use environment variables. Pass this information into the class
as
> an argument instead.

Thanks a lot! I added a module named settings, and moved the POLQA path
constants in there. In this way I don't need to pass it to the constructor and I
avoid to add factory classes. We can leave the responsibility to the settings
module (in the future we could add shell arguments to override the default in
settings). Let me know if you recommend a different design.

https://codereview.webrtc.org/2718063003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores.py:168:
assert len(data) == 2  # Two rows expected (header and values).
On 2017/03/03 05:08:07, kjellander_webrtc wrote:
> Please add a friendly message to be printed if the assert fails.

Done.

[kjellander_webrtc, 2017-03-04 21:21:03]

https://codereview.webrtc.org/2718063003/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/2718063003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores.py:126:
_POLQA_PATH = os.environ['POLQA_PATH']
On 2017/03/03 12:37:56, AleBzk wrote:
> On 2017/03/03 05:08:07, kjellander_webrtc wrote:
> > Please don't use environment variables. Pass this information into the class
> as
> > an argument instead.
> 
> Thanks a lot! I added a module named settings, and moved the POLQA path
> constants in there. In this way I don't need to pass it to the constructor and
I
> avoid to add factory classes. 

I'd say it would be an improvement to pass it into the constructor. That will
make the class more testable and makes it more clear what the class needs
(instead of hiding that information in the class).

> We can leave the responsibility to the settings
> module (in the future we could add shell arguments to override the default in
> settings). Let me know if you recommend a different design.

Since you're still relying on the environment variable in settings.py, I won't
approve unless you can prove there's no other way to solve this. Environment
variables are always a source of error since they tend to be hard to see when
debugging and easy to be missed when trying to reproduce.

Why can't you just have a command line flag that is always passed? If you think
that's inconvenient I'd be happy to accept a reasonable default value for it
(e.g. os.path.join(os.path.expanduser(~), 'polqa') or something like that).
Relying on the use having set POLQA_PATH is far from ideal and it will add extra
maintenance to the bots as well if we would run this as part of the continuous
build.

[AleBzk, 2017-03-06 13:27:17]

Hi Henrik,

Thanks a lot for your comments, now it really looks much better :)

Since I was using environment variables also in another part of the code, I
decided to also fix that part. You may already have seen the changes in
https://codereview.webrtc.org/2715233003/. Briefly, I now have the parameters as
shell arguments, they are passed to a factory class that knows when to use them.
This further simplifies testing and future changes (e.g., getting even more
params from the user).

Thanks a lot for your review,
Alessio

[kjellander_webrtc, 2017-03-06 14:36:39]

https://codereview.webrtc.org/2718063003/diff/40001/webrtc/modules/audio_proc...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores.py
(right):

https://codereview.webrtc.org/2718063003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores.py:135:
self._polqa_tool_path, self._BIN_FILENAME)
How about to check for the existence of this file here?

[AleBzk, 2017-03-06 14:47:02]

On 2017/03/06 14:36:39, kjellander_webrtc wrote:
>
https://codereview.webrtc.org/2718063003/diff/40001/webrtc/modules/audio_proc...
> File
>
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores.py
> (right):
> 
>
https://codereview.webrtc.org/2718063003/diff/40001/webrtc/modules/audio_proc...
>
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores.py:135:
> self._polqa_tool_path, self._BIN_FILENAME)
> How about to check for the existence of this file here?

done

[kjellander_webrtc, 2017-03-06 19:14:27]

Lgtm with comment addressed

https://codereview.webrtc.org/2718063003/diff/60001/webrtc/modules/audio_proc...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores.py
(right):

https://codereview.webrtc.org/2718063003/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores.py:137:
raise Exception('cannot find POLQA tool binary file')
Please add a user defined Exception class, like 
class EvaluationError(Exception):
  pass

Then raise that + include the path in the error message.

[AleBzk, 2017-03-10 11:39:35]

https://codereview.webrtc.org/2718063003/diff/60001/webrtc/modules/audio_proc...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores.py
(right):

https://codereview.webrtc.org/2718063003/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores.py:137:
raise Exception('cannot find POLQA tool binary file')
On 2017/03/06 19:14:27, kjellander_webrtc wrote:
> Please add a user defined Exception class, like 
> class EvaluationError(Exception):
>   pass
> 
> Then raise that + include the path in the error message.

Done.

[AleBzk, 2017-04-03 15:26:15]

The CQ bit was checked by alessiob@webrtc.org

[AleBzk, 2017-04-03 15:26:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2718063003/#ps100001 (title:
"rebase")

[commit-bot: I haz the power, 2017-04-03 15:26:18]

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-04-03 15:29:09]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491233175913430,
"parent_rev": "c533df203cfdfcf29287591ce591e2715269f45e", "commit_rev":
"0deb594b2567808f4185243a57afd830bd201bac"}

[commit-bot: I haz the power, 2017-04-03 15:29:11]

Description was changed from

==========
POLQA evaluation score.

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

to

==========
POLQA evaluation score.

BUG=webrtc:7218
NOTRY=True

Review-Url: https://codereview.webrtc.org/2718063003
Cr-Commit-Position: refs/heads/master@{#17509}
Committed:
https://chromium.googlesource.com/external/webrtc/+/0deb594b2567808f4185243a5...
==========

[commit-bot: I haz the power, 2017-04-03 15:29:13]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/external/webrtc/+/0deb594b2567808f4185243a5...