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.33k stars 417 forks source link

Raise warning when extruding Shoebox #327

Open Chutlhu opened 7 months ago

Chutlhu commented 7 months ago

Dear developers, I just noticed that, when extruding a 2D shoebox, the default absorptions for the floor and the ceiling are set to 0. Since it is very useful to design room is 2D and then extrude them, it would be be useful to have one of following behaviors:

  1. set the new walls' abs. profile to the one used for the other walls and raise a warning
  2. set the new walls' abs. profile to 0 and raise a warning.

With Shoebox class

W, L, H = 5, 4.5, 3.1
room = pra.ShoeBox([W, L], fs=fs, absorption=0.99, max_order=1, sigma2_awgn=0.000000)
print([room.walls[w].absorption for w in range(len(room.walls))])
room.extrude(H)
print([room.walls[w].absorption for w in range(len(room.walls))])

output:

[0.9998999834060669, 0.9998999834060669, 0.9998999834060669, 0.9998999834060669]
[0.9998999834060669, 0.9998999834060669, 0.9998999834060669, 0.9998999834060669, 0.0, 0.0]

With Room class

corners = np.array([[0,0], [0,3], [5,3], [5,1], [3,1], [3,0]]).T  # [x,y]
room = pra.Room.from_corners(corners, absorption=0.99)
print([room.walls[w].absorption for w in range(len(room.walls))])
room.extrude(3.)
print([room.walls[w].absorption for w in range(len(room.walls))])

output:

[0.9998999834060669, 0.9998999834060669, 0.9998999834060669, 0.9998999834060669]
[0.9998999834060669, 0.9998999834060669, 0.9998999834060669, 0.9998999834060669, 0.0, 0.0]
Chutlhu commented 7 months ago

Ah, I noticed now the warning about absorption being deprecated. Maybe I should always materials

fakufaku commented 7 months ago

Thanks for raising this point.

I'm ready between the lines that you spent some time debugging a bug due to this behavior 🙇

The problem is that no default value is always what you want.

What do you think about alternative strategies

  1. make the materials argument mandatory for the extrude method
  2. set to average absorption to 1.0 instead so that the new room is consistent with the 2D one

Also, please use the materials argument instead. absorption has been deprecated for a long time. Beware that the definition of absorption is different from that of energy_absorption of a material.

Chutlhu commented 6 months ago

Dear @fakufaku,

I'm ready between the lines that you spent some time debugging a bug due to this behavior 🙇

Ahah, no, no worries :sweat_smile: For fast prototyping, I usually copy and paste code snippets from the examples and the notebooks. They are such good resources, maybe they should be updated.

What do you think about alternative strategies

  1. make the materials argument mandatory for the extrude method

I think solution 1. Why not solution 2.? Because at this point, it is better to avoid any further use of the absorption argument. Making material mandatory in the extrude method may add some complexity, but it will rise awareness and avoid bugs. Cheers :bow: Thank you for this amazing library