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

Raise warning when extruding Shoebox #327

Open
Chutlhu opened this issue Oct 20, 2023 · 3 comments
Open

Raise warning when extruding Shoebox #327

Chutlhu opened this issue Oct 20, 2023 · 3 comments

Comments

@Chutlhu
Copy link

Chutlhu commented Oct 20, 2023

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
Copy link
Author

Chutlhu commented Oct 20, 2023

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

@fakufaku
Copy link
Collaborator

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
Copy link
Author

Chutlhu commented Oct 23, 2023

Dear @fakufaku,

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

Ahah, no, no worries 😅
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 🙇
Thank you for this amazing library

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

No branches or pull requests

2 participants