Scattering fix - Githubissues

LCAV / pyroomacoustics

Pyroomacoustics is a package for audio signal processing for indoor applications. It was developed as a fast prototyping platform for beamforming algorithms in indoor scenarios.

https://pyroomacoustics.readthedocs.io

MIT License

1.35k stars 419 forks source link

Scattering fix #125

Closed ebezzam closed 4 years ago

ebezzam commented 4 years ago

Thanks for sending a pull request (PR), we really appreciate that! Before hitting the submit button, we'd be really glad if you could make sure all the following points have been cleared.

Please also refer to the doc on contributing for more details. Even if you can't check all the boxes below, do not hesitate to go ahead with the PR and ask for help there.

[ ] Are there docstrings ? Do they follow the numpydoc style ?
[ ] Have you run the tests by doing nosetests or py.test at the root of the repo ?
[ ] Have you checked that the doc builds properly and that any new file has been added to the repo ? How to do that is covered in the documentation.
[ ] Is there a unit test for the proposed code modification ? If the PR addresses an issue, the test should make sure the issue is fixed.
[ ] Last but not least, did you document the proposed change in the CHANGELOG file ? It should go under "Unreleased".

Happy PR :smiley:

fakufaku commented 4 years ago

BTW, the test tests/tests_libroom/test_ray_energy.py is failing. It was depending on the computation of the scattering, so the tests should be fixed to account for the corrected computation.

ebezzam commented 4 years ago

Actually tests/tests_libroom/test_ray_energy.py doesn't use scattering, and it was failing in the original branch. But I can take a look.

But there is a new test that's failing due to the scattering: test_scat_ray_ok.

ebezzam commented 4 years ago

What I notice is that the initial energy is set differently.

ground-truth vs. ray tracing simulation, there is a factor 2 difference which I also see in the resulting histograms

ebezzam commented 4 years ago

From Eq. 5.54 in Dirk's thesis, it seems that it should be multiplied by 2 (instead of 4) as done for the ground-truth histogram. And do we need this (1-cos gamma/2) term?

fakufaku commented 4 years ago

Hi @ebezzam, I'm not completely sure where this PR was left at. Did we solve the factor 2 problem ?

fakufaku commented 4 years ago

Also, as you mentioned to me, the current code does not use scattering for reflections under the ISM model order, which is not correct. I think the fix is very simple.

In pyroomacoustics/libroom_src/room.cpp, line 842, remove the second condition from the if clause (!already_in_ism).

fakufaku commented 4 years ago

BTW, I've fixed a bunch of stuff in next_gen_simulator, you might want to sync the branches.

ebezzam commented 4 years ago

Hi @ebezzam, I'm not completely sure where this PR was left at. Did we solve the factor 2 problem ?

hey @fakufaku, sorry forgot to push that commit. do you agree it should be multiplied by 2 instead of 4?

ebezzam commented 4 years ago

Also, as you mentioned to me, the current code does not use scattering for reflections under the ISM model order, which is not correct. I think the fix is very simple.

In pyroomacoustics/libroom_src/room.cpp, line 842, remove the second condition from the if clause (!already_in_ism).

I think that's one thing that needs to be changed but also attenuate the ISM amplitudes by sqrt(1 - scattering coefficient)

fakufaku commented 4 years ago

Eric, thanks for the PR. I noticed still a few things, like the is_hybrid_sim parameters is not set correctly currently (my fault, always set to True, even for pure ISM), which leads to wrong energy for ISM that doesn't use ray tracing. But I'm merging the PR so that we can move forward.