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

Speech directivity #317

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Conversation

pkendrick
Copy link

@pkendrick pkendrick commented Jun 5, 2023

This PR is an attempt to add frequency dependent source directivity. I have created a new class SpeechDirectivity and adds some examples and unit tests. This is my first PR for pyroomacoustics, let me know if ive missed anything or any issues. Thanks!


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 😃

@fakufaku
Copy link
Collaborator

Hi @pkendrick , thank you so much for the PR!

I think it would be pretty nice to support such sources.

There are however a couple of roadblocks ahead

  1. I am currently integrating another pull request to add support for measured directivites Support for recorded source/mic directivities from DIRPAT database (#259) #302 , I think there is significant code overlap with your own contribution. In particular, I have significantly refactored the compute_rir function. The bad news is that you will probably have to rewrite that part so that it plays nicely with your contribution. The good news is that the refactored version should be a lot simpler to understand.
  2. As part of the previous point, I have changed a little bit the concept of the Directivity object so that when requesting the gains at different angles, it will return the gains for all frequency bands, rather than having a frequency argument. The assumptions is that either the object is frequency independent, uses octave bands (same as simulation), or will return an impulse response.

I am quite advanced in integrating #302 so I don't think it should take too long before it is merged in master. Essentially, it is working and the tests are passing, but I would like to check a little bit more and possibly add extra tests and doc.

@pkendrick
Copy link
Author

Thanks alot for letting me know, that all sounds pretty good. No problem, I am happy to update what to match this work when it is ready. Cheers!

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.

2 participants