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.

Sign up for GitHub

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

Filter deactivated sensors. #2078

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+19 −0
Open

Filter deactivated sensors. #2078

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1732b2a
Filter deactivated sensors.
0mdc Sep 13, 2024
1fda203
Formatting fixes.
0mdc Sep 13, 2024
2e5aec2
Add a config flag to enable sensor filtering.
0mdc Nov 15, 2024
a9dab41
Change doctrings and variable names.
0mdc Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions habitat-lab/habitat/config/default_structured_configs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about cull_unused_visual_sensors

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about a warning comment that explains the dependency issue. You can mention humanoid_detector_sensor and spot_head_stereo_depth_sensor.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I would expect the comment for this flag to go above it, not below it.

Copy link
Contributor Author

@0mdc 0mdc Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

image

image

"""
When 'True', all visual sensors excluded from 'obs_keys' are removed from the simulation.
"""


@dataclass
Expand Down
16 changes: 16 additions & 0 deletions habitat-lab/habitat/core/env.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,22 @@ def __init__(
else:
self.number_of_episodes = None

# Filter out inactive sensors from habitat-sim config.
0mdc marked this conversation as resolved.
Show resolved Hide resolved
if config.gym.cull_visual_sensors:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

habitat-lab/habitat-lab/habitat/tasks/rearrange/rearrange_sim.py

Line 68 in f411876

if len(config.agents) > 1:

UUIDs get changed again when they are passed to sim here:

habitat-lab/habitat-lab/habitat/sims/habitat_simulator/habitat_simulator.py

Line 371 in f411876

sensor_specifications = []

@aclegg3 let's consider this as we refactor agents and sensors.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what's your plan here? Are you going to make a change here before merging this PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eundersander I would then go and merge this as-is.

active_sensors = config.gym.obs_keys
agent_configs = config.simulator.agents
with read_write(agent_configs):
for agent_name, agent_config in agent_configs.items():
active_sim_sensors = {}
for (
sensor_key,
sensor_config,
) in agent_config.sim_sensors.items():
sensor_uuid = f"{agent_name}_{sensor_config.uuid}"
if sensor_uuid in active_sensors:
active_sim_sensors[sensor_key] = sensor_config
agent_config.sim_sensors = active_sim_sensors

self._sim = make_sim(
id_sim=self._config.simulator.type, config=self._config.simulator
)
Expand Down