Skip to content

Simplify PlacementResult#793

Merged
zhx06 merged 3 commits into
mainfrom
zxiao/feature/placement_result_unification
Jun 16, 2026
Merged

Simplify PlacementResult#793
zhx06 merged 3 commits into
mainfrom
zxiao/feature/placement_result_unification

Conversation

@zhx06

@zhx06 zhx06 commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

Summary

Unify PlacementResult and MultiEnvPlacementResult

Detailed description

  • Remove MultiEnvPlacementResult
  • Update related code and tests

@greptile-apps

greptile-apps Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR unifies PlacementResult and MultiEnvPlacementResult by deleting the wrapper class and making ObjectPlacer.place() always return list[PlacementResult] — one element per environment — regardless of whether num_envs is 1 or greater.

  • MultiEnvPlacementResult is fully removed; no remaining references exist in the codebase.
  • Single-env call sites are updated to use tuple-unpacking ((result,) = placer.place(...)) and multi-env sites switch from result.results[i] to result[i].
  • A new test_placement_result.py covers PlacementResult.success in isolation.

Confidence Score: 5/5

Safe to merge — all in-repo callers are updated, MultiEnvPlacementResult has no remaining references, and the simplified list return is consistent throughout.

The change removes a wrapper class and flattens the return type to always be a plain list. Every call site in tests, notebooks, and the pooled placer has been updated. No remaining references to MultiEnvPlacementResult exist anywhere in the repository, and the new test_placement_result.py provides focused coverage of the remaining PlacementResult.success logic.

No files require special attention.

Important Files Changed

Filename Overview
isaaclab_arena/relations/placement_result.py MultiEnvPlacementResult removed; PlacementResult kept unchanged as the single per-env result type.
isaaclab_arena/relations/object_placer.py place() return type simplified from PlacementResult
isaaclab_arena/relations/pooled_object_placer.py isinstance(result, MultiEnvPlacementResult) guard removed; place() result used directly as a list.
isaaclab_arena/tests/test_placement_result.py New unit tests for PlacementResult.success covering required-pass, required-fail, and optional-fail scenarios.
isaaclab_arena/tests/test_heterogeneous_placement.py isinstance(result, MultiEnvPlacementResult) assertions replaced with len(result) checks; result.results[i] replaced with result[i].

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["ObjectPlacer.place(objects, num_envs)"] --> B["_place_ranked() → ranked_results_per_env"]
    B --> C["results_per_env = [env[0] for env in ranked]"]
    C --> D["return results_per_env  ← list[PlacementResult]"]

    D --> E1["Single-env caller\n(result,) = placer.place(...)"]
    D --> E2["Multi-env caller\nfor r in placer.place(..., num_envs=N)"]
    D --> E3["PooledObjectPlacer\nall_layouts = placer.place(..., num_envs=batch)"]

    style D fill:#22c55e,color:#fff
    style E1 fill:#3b82f6,color:#fff
    style E2 fill:#3b82f6,color:#fff
    style E3 fill:#3b82f6,color:#fff
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A["ObjectPlacer.place(objects, num_envs)"] --> B["_place_ranked() → ranked_results_per_env"]
    B --> C["results_per_env = [env[0] for env in ranked]"]
    C --> D["return results_per_env  ← list[PlacementResult]"]

    D --> E1["Single-env caller\n(result,) = placer.place(...)"]
    D --> E2["Multi-env caller\nfor r in placer.place(..., num_envs=N)"]
    D --> E3["PooledObjectPlacer\nall_layouts = placer.place(..., num_envs=batch)"]

    style D fill:#22c55e,color:#fff
    style E1 fill:#3b82f6,color:#fff
    style E2 fill:#3b82f6,color:#fff
    style E3 fill:#3b82f6,color:#fff
Loading

Reviews (4): Last reviewed commit: "remove multi-env" | Re-trigger Greptile

Comment thread isaaclab_arena/relations/placement_result.py Outdated
Comment thread isaaclab_arena/relations/placement_result.py Outdated
@zhx06 zhx06 force-pushed the zxiao/feature/placement_result_unification branch 2 times, most recently from 375482d to d1aa47c Compare June 16, 2026 16:56
@zhx06 zhx06 changed the title Unification of PlacementResult Simplify PlacementResult Jun 16, 2026

@alexmillane alexmillane left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the clean up!

@zhx06 zhx06 force-pushed the zxiao/feature/placement_result_unification branch from d1aa47c to 90676a2 Compare June 16, 2026 19:01
@zhx06 zhx06 merged commit e080f87 into main Jun 16, 2026
11 of 12 checks passed
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

Successfully merging this pull request may close these issues.

2 participants