Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add AnechoicRoom class #224

Merged
merged 30 commits into from
Nov 10, 2022
Merged

Add AnechoicRoom class #224

merged 30 commits into from
Nov 10, 2022

Conversation

duembgen
Copy link
Contributor

@duembgen duembgen commented Jun 3, 2021

This pull request reopens #160, now against the new version of pyroomacoustics.

This will close issue #13.

I am working on polishing it and will remove the "WIP" tag when done.

  • 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".

@fakufaku
Copy link
Collaborator

fakufaku commented May 1, 2022

Hi @duembgen ! What is the status on this pull request ? If it is good, I could just pull it in. The tests are failing now, so maybe a merge or something is needed.

Copy link
Collaborator

@fakufaku fakufaku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could not find the definition of the AnechoicRoom anymore ? Is the code in sync ?

README.rst Outdated
@@ -158,7 +158,7 @@ The minimal dependencies are::

where ``Cython`` is only needed to benefit from the compiled accelerated simulator.
The simulator itself has a pure Python counterpart, so that this requirement could
be ignored, but is much slower.
be ignored, but is much slower. For the Cython implementation to work, the user should have Eigen installed.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, if you check the repo with the --recursive option, it should pull the eigen library too. I don't think the note is needed. This is covered in CONTRIBUTING.rst.

@duembgen
Copy link
Contributor Author

duembgen commented May 2, 2022

Hi @fakufaku thank you for your comments! I am finalizing the pull request now. Will remove the Eigen comment if it is fixed by doing a recursive clone. Do you have any preference on how / if to mention the AnechoicRoom class in the documentation? I added another section called Free-field simulation in the room doc file, but happy to change that.

@duembgen
Copy link
Contributor Author

duembgen commented May 2, 2022

Regarding the CHANGELOG, would you like me to be more specific than Added AnechoicRoom class?

Copy link
Collaborator

@fakufaku fakufaku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool, LGTM! Thanks, it makes it even easier to use in simple scenarios!

There were just a couple of things that I have added in comments. I can merge as soon as you are done.


def __init__(
self,
dim,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this dim=3 by default ? I would like to move away from 2D room simulation in the long term :)

=====================

You can also use this package to simulate free-field sound propagation between a set of sound sources and a set of microphones, without considering room effects. To this end, you can use the :py:obj:`pyroomacoustics.room.AnechoicRoom` class, which simply corresponds to setting the maximum image image order of the room simulation to zero. This allows for early development and testing of various audio-based algorithms, without worrying about room acoustics at first. Thanks to the modular framework of pyroomacoustics, room acoustics can easily be added, after this first testing stage, for more realistic simulations.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add a link to the relevant example in the paragraph ?

Also, a short code snippet would be awesome, but I leave that to you to decide! If you don't feel like it, don't do it 😄

Copy link
Contributor Author

@duembgen duembgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the review!
I created an example script in the examples folder and added a corresponding code snippet to the docs. If you agree with all the changes you can merge this pull request.

@EliasBrohammer
Copy link

sorry for bursting into your conversation:
Just yesterday I created a script using the idea from issue #13 (max_order=0 for the room creation).
I will try the new room as soon as it's ready and give you feedback!

@duembgen duembgen changed the title WIP: Add AnechoicRoom class Add AnechoicRoom class May 3, 2022
Copy link
Collaborator

@fakufaku fakufaku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of things left!

Simulating Direction-of-Arrival Estimation in Free Field
--------------------------------------------------------

As a simple usage example, assume you want to localize a sound source emitting white noise, using a square microphone array. If you can neglect room effects (e.g. you operate in an anechoic room or outdoors), or if you simply want to test your algorithm in the best-case scenario, you can use the :py:obj:`pyroomacoustics.room.AnechoicRoom` class. The below code shows an example using the popular MUSIC algorithm for DOA estimation. The example is exctracted from `./examples/doa_anechoic_room.py`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exctracted -> extracted

# run the simulation
room.simulate()

# create frequency-domain input for DOA algorithms
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the example! But I feel it is a bit long. Maybe we could leave the DOA part out and use some hard-coded value for the parameters (e.g. pra.AnechoicRoom(fs=16000)). Without the DOA we also won't need n_fft.

@@ -2365,9 +2399,15 @@ def get_wall_by_name(self, name):
"""
Returns the instance of the wall by giving its name.

:arg name: (string) name of the wall
Parameters
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing the formatting in different places!

@EliasBrohammer
Copy link

Just wanted to ask, how the status on this pull-request is? :)

@duembgen
Copy link
Contributor Author

@fakufaku I have simplified the example, I hope that's roughly what you had in mind. Feel free to change anything to your liking and merge as soon as you are satisfied.
@EliasBrohammer sorry for the late reply.

@fakufaku fakufaku merged commit c30a0a6 into LCAV:master Nov 10, 2022
@fakufaku
Copy link
Collaborator

@duembgen Sorry it took me so long to merge this! Thanks for your contribution!!! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants