Skip to content

refactor!: replace DepthTo3DLocations sensor args with SensorConfig sequence#856

Open
TimothyAlexisVass wants to merge 7 commits intothousandbrainsproject:mainfrom
TimothyAlexisVass:refactor/depth-to-3d-per-sensor-config
Open

refactor!: replace DepthTo3DLocations sensor args with SensorConfig sequence#856
TimothyAlexisVass wants to merge 7 commits intothousandbrainsproject:mainfrom
TimothyAlexisVass:refactor/depth-to-3d-per-sensor-config

Conversation

@TimothyAlexisVass
Copy link
Copy Markdown
Contributor

@TimothyAlexisVass TimothyAlexisVass commented Mar 19, 2026

Follow-up to

@tristanls-tbp

suggestion (non-blocking, different pull request): There's an opportunity here to only accept Sequences. It is trivial to specify a sequence of one, so I'd rather we do that in the code then having to deal with if isinstance(thing, (int, float)):.

suggestion (non-blocking, different pull request): Seeing how we multiply zooms and hfovs to match length of sensor_ids, points to that we could better configure things by accepting a Sequence[SensorConfig] or something similar that has all the data we need.

Summary

This follow-up PR removes legacy parallel/union constructor inputs from DepthTo3DLocations and enforces a single, typed configuration path via Sequence[SensorConfig].

Changes

  • DepthTo3DLocations.__init__ now requires sensor_configs: Sequence[SensorConfig].
    • The legacy arguments sensor_ids, resolutions, zooms, and hfov were removed.
    • The constructor now accepts only typed sensor configuration objects and drops scalar acceptance for zoom/FOV.
  • Main changes are in src/tbp/monty/frameworks/environment_utils/transforms.py

Most of the files changed are .yaml configs and snapshots, updated to the new SensorConfig contract:

% git diff main --name-only -- . ':(exclude)*.yaml'
src/tbp/monty/frameworks/environment_utils/transforms.py
src/tbp/monty/frameworks/environments/two_d_data.py
tests/unit/frameworks/environment_utils/habitat_transform_test.py

Scope / Behavior

  • In-repo callers already using SensorConfig continue to work.
  • This PR is a stricter, typed follow-up and intentionally removes old construction paths for DepthTo3DLocations.

⚠️ Follow-up / Risk

  • Breaking change for any external code that still calls DepthTo3DLocations with old positional args (sensor_ids, resolutions, zooms, hfov) and is not migrated to SensorConfig.
    If any unexpected callers require backwards-compatibility to the legacy arguments, that is already prepared and we could drop commit 305b918.

@TimothyAlexisVass TimothyAlexisVass marked this pull request as ready for review March 19, 2026 10:14
@tristanls-tbp tristanls-tbp self-requested a review March 23, 2026 20:52
@tristanls-tbp tristanls-tbp self-assigned this Mar 23, 2026
@tristanls-tbp tristanls-tbp added the triaged This issue or pull request was triaged label Mar 23, 2026
@tristanls-tbp tristanls-tbp changed the title refactor: replace DepthTo3DLocations sensor args with SensorConfig sequence refactor!: replace DepthTo3DLocations sensor args with SensorConfig sequence Mar 23, 2026
@tristanls-tbp
Copy link
Copy Markdown
Contributor

tristanls-tbp commented Mar 23, 2026

note: Added breaking change ! indicator to the PR title since this is a breaking change and requires users to change how they configure Monty.

@TimothyAlexisVass
Copy link
Copy Markdown
Contributor Author

@tristanls-tbp If we want backwards compatibility instead of making this a breaking change, I did prepare that.
I wasn't sure what the convention is in this project. So, if we want it, we can drop the commit that removed that preparation:
Remove backwards compatibility for external callers

@tristanls-tbp
Copy link
Copy Markdown
Contributor

note: Apologies, but I intend to delay review of this and other config-altering PRs until after #857 is merged. I know that #857 wasn't around when you submitted the PR, but it's big and took a lot of validation work.

@tristanls-tbp
Copy link
Copy Markdown
Contributor

Hi @TimothyAlexisVass, ok, we reworked how we do configurations in #857, which is now merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

triaged This issue or pull request was triaged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants