Filter deactivated sensors. #2078
Conversation
facebook-github-bot
added
the
CLA Signed
0mdc
changed the title
0mdc
force-pushed
the
0mdc/filter_deactivated_sensors
branch
from
e2e87eb
to
1fda203
Compare
0mdc
changed the title
| @@ -1887,6 +1887,10 @@ class GymConfig(HabitatBaseConfig): | |||
| action_keys: Optional[List[str]] = None | |||
| achieved_goal_keys: List = field(default_factory=list) | |||
| desired_goal_keys: List[str] = field(default_factory=list) | |||
| cull_visual_sensors: Optional[bool] = False | |||
There was a problem hiding this comment.
How about cull_unused_visual_sensors
There was a problem hiding this comment.
How about a warning comment that explains the dependency issue. You can mention humanoid_detector_sensor and spot_head_stereo_depth_sensor.
There was a problem hiding this comment.
Also I would expect the comment for this flag to go above it, not below it.
There was a problem hiding this comment.
Also I would expect the comment for this flag to go above it, not below it.
You right that in this file, only class docstrings are under the symbols, so I'll change accordingly.
However, comments above the attributes aren't recognized as docstrings.
habitat-lab/habitat/core/env.py
Outdated
| @@ -111,6 +111,22 @@ def __init__( | |||
| else: | |||
| self.number_of_episodes = None | |||
|
|
|||
| # Filter out inactive sensors from habitat-sim config. | |||
| if config.gym.cull_visual_sensors: | |||
There was a problem hiding this comment.
Doing this cull logic here in Env.__init__... I wonder if this is too late and not ideal. For example, what about our call to envs.initialize_batch_renderer which also reads sim_sensors? Will it see the updated sim_sensors? For reference, when I accidentally re-implemented this recently, I put the culling in default.py patch_config, which is much earlier in our loading path. I don't know if that's better.
There was a problem hiding this comment.
I agree. There is some unwrangling to do here.
This pattern of changing configs on-the-fly is causing us headaches with sensors. @aclegg3 and @jturner65 are currently working on refactoring them.
For example:
rearrange_sim changes the UUIDs here:
UUIDs get changed again when they are passed to sim here:
@aclegg3 let's consider this as we refactor agents and sensors.
There was a problem hiding this comment.
So what's your plan here? Are you going to make a change here before merging this PR?
There was a problem hiding this comment.
Sensor refactor won't be "done" for a few weeks. I suggest you move forward with a stop-gap if you need this change to be around soon. If this is just living on a branch or not really needed in the short term then you could wait. A similar mechanism is planned to be included in the sensor refactor.
There was a problem hiding this comment.
@eundersander I would then go and merge this as-is.
Motivation and Context
This changeset adds a flag to filter out unused sensors from simulation.
In lab, only the sensors listed in
gym.obs_keyscan be read. However, all other sensors are still rendered.Therefore, when pulling agents from configs, all their sensors are rendered.
For example, if a human and spot are instantiated, the following sensors are rendered:
Note:
This breaks composite sensors. For example, a "human sensor" may depend on a RGB camera, but the filtering will cull out the RGB camera because it's not listed in
gym.obs_key. For this reason, the flag is disabled by default. A sensor dependency graph will be implemented to work around this problem.Performance
Here's the difference on a single-learn HITL session with 3 visual sensors.
On a reference single-learn HITL application, this improves the framerate from
9.1FPS to11.3FPS.Performance gains were reported to be significantly higher in other use cases such as training.
How Has This Been Tested
Tested locally and profiled.
Running unit tests on CI.
Types of changes
Checklist