Deterministic Object Placement

[zhx06]

Summary

Deterministic Object Placement

Detailed description

• Add seed and random number generator to make optimized layouts deterministic
• Add several related tests to guarantee the correctness

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This PR introduces deterministic object placement by adding a seed_offset parameter to ObjectPlacer.place() and managing seed streams within PooledObjectPlacer. The implementation ensures reproducibility across pool refills and sampling operations, backed by 10 comprehensive tests.

Findings

🔵 Suggestion: In pooled_object_placer.py line 50, the change from raise ValueError(...) to assert pool_size >= 1 is a deliberate design choice. However, note that asserts can be disabled with python -O, which would skip this validation in optimized builds. If this is intentional (fail-fast during development only), the current approach is fine. If this invariant should always be enforced, consider keeping ValueError.

🔵 Suggestion: The seeded random.Random(placer_params.placement_seed) instance (line 54) is correctly used for sample_with_replacement. When placement_seed=None, random.Random(None) seeds from system entropy, which gives the desired non-deterministic behavior. This is correct.

🔵 Suggestion: The test file imports pytest at line 8 but only uses it in the final test (test_pooled_placer_rejects_pool_size_below_one). This is fine, but ensure the test suite dependencies include pytest.

✅ The seed_offset bookkeeping correctly advances only when placement_seed is not None, avoiding phantom seed-stream tracking for unseeded runs.

✅ Tests thoroughly cover: identical seeds produce identical layouts, different seeds diverge, refill advances the seed stream, unseeded mode works without crashes, no dict aliasing, count > pool_size works, and the pool_size validation.

Verdict

Ship it 🚀

Clean implementation with solid test coverage. The suggestions above are minor style considerations; the core logic is correct and well-documented.

[greptile-apps]

Greptile Summary

This PR makes ObjectPlacer and PooledObjectPlacer fully deterministic by threading a seed_offset through successive place() calls so each batch uses a non-overlapping seed range, and by replacing the module-level random.choices with a seeded random.Random instance for sample_with_replacement.

• object_placer.py: A keyword-only seed_offset parameter is added to place(); each candidate seed becomes placement_seed + seed_offset + candidate_idx, ensuring refill batches never replay the initial seed range.
• pooled_object_placer.py: _rng = random.Random(placement_seed) and _next_seed_offset track the sampling and solver seed streams independently; the raise ValueError guard on pool_size was changed to an assert, which is silently disabled under Python's -O flag.
• test_object_placer_reproducibility.py: Eight new tests cover same-seed reproducibility, cross-refill continuity, and unseeded non-determinism; the validation test catches AssertionError rather than ValueError, coupling it to the implementation mechanism.

Confidence Score: 3/5

Safe to merge after restoring the ValueError guard and aligning the test to catch ValueError instead of AssertionError.

The core seeding logic is correct and well-tested. The only substantive change that could affect production behavior is the replacement of raise ValueError with assert pool_size >= 1 in PooledObjectPlacer.__init__. Any deployment running Python with -O will silently skip that guard, and users who pass an invalid pool_size will get a confusing RuntimeError rather than a clear ValueError. The test also locks in this behavior by catching AssertionError instead of ValueError.

pooled_object_placer.py and the corresponding test in test_object_placer_reproducibility.py.

Important Files Changed

Filename	Overview
isaaclab_arena/relations/pooled_object_placer.py	Adds _rng (seeded random.Random) and _next_seed_offset for deterministic pool refills; replaces random.choices with self._rng.choices. The raise ValueError guard on pool_size was downgraded to assert, which is silently skipped under -O.
isaaclab_arena/relations/object_placer.py	Adds keyword-only seed_offset parameter to place(). Seed per candidate is now placement_seed + seed_offset + candidate_idx, keeping successive batches on non-overlapping seed ranges. Change is minimal and correct.
isaaclab_arena/tests/test_object_placer_reproducibility.py	Eight new reproducibility tests for PooledObjectPlacer. The test_pooled_placer_rejects_pool_size_below_one test catches AssertionError instead of ValueError, coupling it to the implementation mechanism; and test_pooled_placer_unseeded_does_not_crash_and_diverges makes a non-deterministic assertion that is theoretically flaky.

_{Reviews (1): Last reviewed commit: "increase test readability" | Re-trigger Greptile}

[greptile-apps]

Using assert for constructor argument validation is problematic because Python disables all assert statements when run with the -O (optimize) flag. In that mode, a pool_size=0 call will silently pass this check, then max_placement_attempts * 0 = 0 candidates are generated, and the pool initialization will fail later with a generic RuntimeError("Placement pool failed to produce any valid layouts") — losing the precise error message. The original ValueError is the idiomatic Python choice for invalid argument values in a public __init__.

Suggested change

	assert pool_size >= 1, f"pool_size must be >= 1, got {pool_size}"
	if pool_size < 1:
	raise ValueError(f"pool_size must be >= 1, got {pool_size}")

[greptile-apps]

This test is coupled to the implementation mechanism (AssertionError) rather than the semantic contract (ValueError). If the validation is restored to a ValueError (which is idiomatic for invalid constructor arguments), this test would incorrectly pass the assert but then fail with the wrong exception type. It should catch ValueError to verify the public API contract.

Suggested change

	with pytest.raises(AssertionError, match="pool_size must be >= 1"):
	PooledObjectPlacer(objects=list(_create_test_objects()), placer_params=placer_params, pool_size=0)
	with pytest.raises(ValueError, match="pool_size must be >= 1"):
	PooledObjectPlacer(objects=list(_create_test_objects()), placer_params=placer_params, pool_size=0)

[greptile-apps]

Non-deterministic test asserting non-determinism

test_pooled_placer_unseeded_does_not_crash_and_diverges asserts that two independently unseeded pools produce at least one differing sample across 20 draws. While this is overwhelmingly likely in practice, it is not guaranteed: if the gradient solver converges to the same global minimum from two different random inits, all positions could coincide and the assertion fires incorrectly. Consider replacing the divergence assertion with a no-crash smoke test.