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

When I try room.compute_rir() again,is it intended that the ISM or Ray-Tracing will not be re-modeled? #263

Open
AlanLiudx opened this issue Jun 15, 2022 · 5 comments

Comments

@AlanLiudx
Copy link

AlanLiudx commented Jun 15, 2022

Hello faku @fakufaku and thanks for your continuous contribution! I'm new here to learn multi-channel acoustic scene simulation, and I found something weird when trying to compute rir:

def compute_rir(self):
        """
        Compute the room impulse response between every source and microphone.
        """

        if self.simulator_state["ism_needed"] and not self.simulator_state["ism_done"]:
            self.image_source_model()

        if self.simulator_state["rt_needed"] and not self.simulator_state["rt_done"]:
            self.ray_tracing()

        self.rir = []

In the code block above, I see if I change the parameters in the existing ISM or RT model such as max_order(for instance, I first try max_order=0 to generate anechoic speech and then set different sets of RT60 and generate reverberated speech), the ISM or RT will not be re-modeled because self.simulator_state["ism_done"] has been set True in the end of the method of self.image_source_model() or self.ray_tracing().
I wonder if that is a feature or a bug. I think if I want to change nothing in the room but the reverberation time I need, there's no means for me to create another room for another set of speech. Of course I can manually reset self.simulator_state["ism_done"], but seemingly in this way I break the encapsulation of the Room class.
Could you please give me an explanation? I would be grateful for that.

@AlanLiudx AlanLiudx changed the title When I try room.compute_rir() again,is it intend that the ISM or Ray-Tracing will not be re-modeled? When I try room.compute_rir() again,is it intended that the ISM or Ray-Tracing will not be re-modeled? Jun 15, 2022
@fakufaku
Copy link
Collaborator

Hi @AlanLiudx , yes this is more a feature than a bug. There is a lot of states in the internals, and because you can call intermediate functions (such as compute_rir, like you are trying to do), it is difficult to know when re-initialization is important. Right now reseting things in self.simulator_state is the way to go, although not ideal, as you point out. I have been thinking adding a reset method to the room object to take care of that.

@AlanLiudx
Copy link
Author

Thank you for your explanation! But actually I read the source code after I found my test code hadn't acted as I 'd expected, and what if a beginner simulates the room impulse and finds nothing changes, getting confused? Perhaps some hints should be added in the examples or documentations. Thank you!

@fakufaku
Copy link
Collaborator

You are totally correct, this is far from an ideal situation... 😰

@AlanLiudx
Copy link
Author

AlanLiudx commented Jun 17, 2022

I tried to implement a reset_simulator_state method outside the class (outside the source code room.py) without doc. Perhaps in this way a reset method can be tried to be implemented into the source?

from __future__ import division, print_function
import pyroomacoustics
from pyroomacoustics import Room
import numpy as np


import math
import warnings

import numpy as np
import scipy.spatial as spatial

def reset_simulator_state(self):
    self.simulator_state["ism_needed"]=True
    self.simulator_state["rt_needed"]=False
    self.simulator_state["ism_done"]=False
    self.simulator_state["rt_done"]=False

Room.reset_simulator_state=reset_simulator_state

@fakufaku
Copy link
Collaborator

Thanks for sharing! I think the values of ism_needed and rt_needed depend on the options provided when constructing the object.
Another solution would be to not have intermediate methods (like compute_rir) and make everything be done by simulate. Then, at then end of the simulate methods, the values of ism_done and rt_done can be re-initialized.

I think I had originally done that because in some cases we want to keep the same RIR, but change only the source signals. In this case we don't want to re-run compute_rir everytime.

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