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

Randomized image method #250

Closed orchidas closed 2 years ago

orchidas commented 2 years ago
orchidas commented 2 years ago

All requested changes have been added. No plot or print statements in test, new file in examples folder and docstrings updated in room.py.

fakufaku commented 2 years ago

@orchidas Thanks for the update! I think we are almost there! If you could just do the following modifications that would be fantastic.

  1. The test do not pass due to the function test_sweep_measure. Pytest/nose will just run all the functions/files that have test_ or a variation thereof in the name. These functions should not take an mandatory arguments. This one is not a test function as far as I can tell, so could you please rename it to something that does not trigger a test, e.g., measure_sweeping_echo, or just call ssf = met.sweeping_echo_measure(rir,fs) directly from the other functions.
  2. Run black to make the formatting consistent (version >= 22)
    pip install black
    black pyroomacoustics
  3. In the example file, it would be great if you can remove all the intermediate plt.show() and just put one at the end, so that we can see all the figures at once. Also, it would make the result much easier to understand if you add some titles to the figures, e.g. "without random ISM", "with random ISM", etc.

For my own education, in the example, we can still some sweeping echo in the lower frequencies when using the random ISM. Would this disappear if we use a larger displacement ? Also, the amplitude in the regular ISM is much larger than the random one. Is it due to the all the echoes arriving at the same time ?

Cheers!

fakufaku commented 2 years ago

This is looking pretty good, all the tests are passing! There is just the black formatting that seems broken. It doesn't show in the test due to some misconfiguration on my part. I think the standard formatting has changed recently so you should make sure you have the latest version of black before running (>v22). Once this is done I will merge this in the main branch!

orchidas commented 2 years ago

Hi Robin, I downloaded the latest version of black and re-did the formatting. I got this error error: cannot format pyroomacoustics/libroom_src/ext/eigen/scripts/relicense.py: Cannot parse: 57:18: print "SKIPPED", filename Python 2 support was removed in version 22.0.

Regarding your other questions, I am in touch with Enzo and will get back to you shortly.

fakufaku commented 2 years ago

Hi @orchidas , thank you very much. You do not have to worry about the file pyroomacoustics/libroom_src/ext/eigen/scripts/relicense.py, it part of a dependency, not the pyroomacoustics codebase.

fakufaku commented 2 years ago

Finally merged! Sorry it took so long!