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

Room.add_microphone_array() loses array directivity when using MicrophoneArray class #382

Closed
mmjamm opened this issue Nov 21, 2024 · 4 comments · Fixed by #385
Closed

Room.add_microphone_array() loses array directivity when using MicrophoneArray class #382

mmjamm opened this issue Nov 21, 2024 · 4 comments · Fixed by #385
Assignees
Labels

Comments

@mmjamm
Copy link

mmjamm commented Nov 21, 2024

Lovely library! Here's a small bug I stumbled upon.

Room.add_microphone() method accepts arraylike or instance of MicrophoneArray for the first parameter.
When using MicrophoneArray with a defined directivity, the directivity is lost.

Steps to reproduce:

from pyroomacoustics import AnechoicRoom
from pyroomacoustics.beamforming import MicrophoneArray
from pyroomacoustics.directivities import FigureEight, DirectionVector
import numpy as np

fs = 16000
room = AnechoicRoom(fs=fs)

mic_locs = np.c_[
    [6.3, 4.87, 1.2]
]

dir_obj = FigureEight(
    orientation=DirectionVector(azimuth=90, colatitude=15, degrees=True)
)

mic_array = MicrophoneArray(mic_locs, fs, [dir_obj])

room.add_microphone_array(mic_array)
print(f"Directivity, case 1: {room.mic_array.directivity}")

room.add_microphone_array(mic_locs, [dir_obj])
print(f"Directivity, case 2: {room.mic_array.directivity}")

Output of the above code:

Directivity, case 1: [None]
Directivity, case 2: [None, <pyroomacoustics.directivities.analytic.FigureEight object at 0x7fd48955b700>]

I think the bug is in (room.py) add_microphone_array where it assumes that a directivity function parameter is always supplied. IMO it would make more sense to use the directivity already contained in the MicrophoneArray object:

if not isinstance(mic_array, MicrophoneArray):
   # if the type is not a microphone array, try to parse a numpy array
   mic_array = MicrophoneArray(mic_array, self.fs, directivity)
else:
   # if the type is microphone array
   mic_array.set_directivity(directivity)

Version used to reproduce: 0.8.2

@fakufaku
Copy link
Collaborator

Thanks for catching this. You are totally right and this is a bug.

@fakufaku
Copy link
Collaborator

fakufaku commented Dec 7, 2024

So after checking, I recalled that the current API was designed so that you can provide the microphone array information as either

  1. A MicrophoneArray object, that should contain microphone locations and directivities.
  2. A pair of numpy array containing the locations, and a list of corresponding directivities (the directivity parameter of the method).

I will add a check that makes the method fail when mic_array is a MicrophoneArray object and directivity is not None.

@fakufaku fakufaku self-assigned this Dec 7, 2024
@fakufaku fakufaku linked a pull request Dec 7, 2024 that will close this issue
5 tasks
@fakufaku fakufaku added the bug label Dec 7, 2024
fakufaku added a commit that referenced this issue Dec 7, 2024
#385)

Fixes issue #382: When providing a ``MicrophoneArray`` object with directivity to ``Room.add_microphone_array``, the directivity was dropped from the object.
@fakufaku
Copy link
Collaborator

fakufaku commented Dec 7, 2024

Thanks for your patience. This should be now fixed in master.
I'm trying to fix a couple of other issue before updating pypi.

@mmjamm
Copy link
Author

mmjamm commented Dec 13, 2024

The change looks good. Thanks for handling this issue and for the great library!

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

Successfully merging a pull request may close this issue.

2 participants