[MISC] Reduce indentation in narrowphase.py and save 89 lines#2790
Open
hughperkins wants to merge 13 commits into
Open
[MISC] Reduce indentation in narrowphase.py and save 89 lines#2790hughperkins wants to merge 13 commits into
hughperkins wants to merge 13 commits into
Conversation
For every `if cond: <large body>` that wraps the tail of a `for` loop and where the body contains further nested control flow, invert the condition and use `continue` to skip ahead. This collapses one level of nesting on the body and removes no other code. 17 sites flattened, picked from a survey of all `if` statements whose body covers >=20 lines and contains at least one nested `if`/`for`/`while`. Sites where the body was a flat sequence (no nested control flow) were left alone -- inverting+continuing there only renames the guard without reducing depth. Functions whose max AST nesting depth dropped: func_narrow_phase_nonconvex_vs_nonterrain 12 -> 9 func_contact_mpr_terrain 10 -> 8 func_contact_convex_convex_sdf 7 -> 5 _func_multicontact_mpr 6 -> 4 func_contact_edge_sdf 6 -> 4 Lines at >=8 levels of nesting drop ~38% (381 -> 237); lines at >=11 levels drop ~61% (98 -> 38). File line count: 2491 -> 2482. `func_convex_convex_contact`'s max depth (14) is unchanged because its outermost wrap `if not (PLANE and BOX): <whole body>` is a function-level guard, not a loop tail -- flattening it requires return-inside-if support in qd.func, which is a separate work item. Two multi-line conditions (sites at lines 442 and 2045) were negated by hand via De Morgan, preserving their multi-line layout. All other sites are single-line condition flips. File was syntax-checked after every mutation; semantics are preserved by construction for the `continue` substitution and by inspection for the negations.
hughperkins
changed the title
ifs in narrowphase.py![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76ba053279
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+2461
to
+2462
| if is_col: | ||
| continue |
There was a problem hiding this comment.
When vertex-SDF detection misses, this refactor now reaches if is_col: with is_col still false and skips the old func_contact_edge_sdf call entirely; if a later perturbation set is_col true, the added continue also makes the following func_add_contact unreachable. For nonconvex-vs-nonterrain pairs whose collision is only detected by the edge-edge SDF fallback, this silently drops the contact and lets interpenetrations pass through.
Useful? React with 👍 / 👎.
…rrain The previous flatten commit (9afc416) inadvertently deleted the entire edge-edge SDF fallback block in func_narrow_phase_nonconvex_vs_nonterrain. The flatten script's header-end detector looked for a line literally ending in `:`, but the original guard if not is_col: # check edge-edge if vertex-face is not detected ends in `detected` (with the `:` followed by an inline comment). The search walked past the real header, treated the next 23 lines (the edge-SDF pose extraction, the `func_contact_edge_sdf` call, and the inner `if is_col:`) as part of the "header", and silently dropped them. What survived the corruption: the `func_add_contact` call alone, placed inside the new `if is_col: continue` body where it was both unreachable and orthogonal to the intended semantics. Effect on physics: for nonconvex-vs-nonterrain geom pairs where vertex-SDF detection misses the contact but edge-SDF would catch it, the contact was silently dropped and interpenetration could pass through. Reported by chatgpt-codex-connector on PR Genesis-Embodied-AI#2790 (Genesis-Embodied-AI#2790 (comment)). This commit restores the edge-SDF block in the correctly-flattened form, i.e. the outer `if not is_col: <body>` is replaced by `if is_col: continue` followed by the body at one shallower indent (including the inner `if is_col: func_add_contact(...)`). Verified that: - the file parses, - ruff format / check are clean, - the count of every collision-detection / func_add_contact call matches the pre-flatten state (37 calls total, all 16 names identical).
…utodiff) Quadrants reverse-mode autodiff (`auto_diff_common.h:visit@407`) does not support `continue` inside differentiable kernels. The two `if i_c >= ...: continue` flattens introduced by 9afc416 caused both test_diff_contact[gpu] and test_diff_contact[cpu] to fail at `func_narrow_phase_diff_convex_vs_convex.grad(...)` with `RuntimeError: Not supported`. Restored the original `if i_c < ...:` form in both for-loops of the kernel, with a short comment at each site explaining why the flatten was not applied here. This loses one level of indent reduction on ~25 lines in each loop, but is required for autodiff correctness. Reported by tests/test_grad.py::test_diff_contact on cluster run of hp/narrowphase-flatten-today @ 213309e.
Previously:
if not repeated:
if penetration > -tolerance:
<16-line body>
The two guards have no sibling statements between them, so they can be
collapsed to a single condition `(not repeated) and penetration > -tolerance`
without semantic change. Body dedents by one visible indent level.
In `func_convex_convex_contact`, inside the `else:` branch of `if qd.static(static_rigid_sim_config.requires_grad)` at line 797, a second `if qd.static(static_rigid_sim_config.requires_grad):` appeared at the old line 832 guarding `penetration = gjk_state.diff_penetration[...]`. Because both checks are `qd.static`, the inner check is statically false along this code path -- the assignment was dead code. Removed the dead `if` and its body; `func_add_contact` continues to use the per-batch `penetration` set just before the outer `if is_col:` block. Replaced with a short comment explaining why no diff_penetration read is needed on this path. Drops the file's deepest visible indent level from 13 to 12 (this was the only level-13 statement).
In `func_convex_convex_contact`'s `is_col` handler, the `if qd.static(requires_grad): ... else: if multi_contact_flag: ... else: ...` chain was written with a manually-nested `else:` containing another `if`, which adds one indent level to the body of the inner `if`. Rewriting as `elif multi_contact_flag:` flattens it without changing semantics. Body of the multi_contact_flag branch dedents by one visible indent level (24 lines).
The outer `if qd.static(ccd_algorithm != MJ_MPR):` had `if prefer_gjk:` as its sole body statement, so the two guards collapse to a single `if qd.static(ccd_algorithm != MJ_MPR) and prefer_gjk:` without any behavioral change. The 111-line GJK code block (diff/non-diff dispatch + is_col handling + multi-contact processing) dedents by one visible indent level. This is the single largest pre-feature indent reduction available on func_convex_convex_contact.
The two guards had no sibling statements between them, so they merge to a single `if qd.static(ccd_algorithm == MPR) and penetration > tolerance:` without semantic change. Body (the multi-line `prefer_gjk = ...` assignment) dedents by one visible indent level.
…tart The two guards had only the warm-start-fallback comment block between them, so they collapse to `if i_mpr == 1 and qd.static(not enable_mujoco_compatibility):` without semantic change. The comment moves above the merged condition. Inner `if (i_detection == 0) and ...:` body dedents by one visible indent level.
The remaining nested `if (i_detection == 0) and not is_col and is_mpr_guess_direction_available:` was the sole body of the previous merged guard, so all three conditions collapse into a single `if` block. The three-statement body (reset warm-start) dedents by one further visible indent level. Combined with the previous commit, this peels two indent levels off the MPR warm-start-fallback block (visible level 10 -> 8 for normal_ws / is_mpr_* resets).
Several wrapped function calls and signatures fit on one line under the project's 120-column limit but were kept multi-line by ruff's magic-trailing-comma rule. After the indent reductions on this branch (continue-flattens + nested-if merges + elif rewrites) many of these calls now have enough horizontal room to collapse, so we re-format with `ruff format --config format.skip-magic-trailing-comma=true` and remove the now-unnecessary trailing commas. Pure formatting change; ruff format / ruff check both pass.
hughperkins
changed the title
|
🔴 Benchmark Regression Detected ➡️ Report |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
For every
if cond: <large body>that wraps the tail of aforloop and where the body contains further nested control flow, invert the condition and usecontinueto skip ahead. This collapses one level of nesting on the body and removes no other code.17 sites flattened, picked from a survey of all
ifstatements whose body covers >=20 lines and contains at least one nestedif/for/while. Sites where the body was a flat sequence (no nested control flow) were left alone -- inverting+continuing there only renames the guard without reducing depth.Functions whose max AST nesting depth dropped:
func_narrow_phase_nonconvex_vs_nonterrain 12 -> 9
func_contact_mpr_terrain 10 -> 8
func_contact_convex_convex_sdf 7 -> 5
_func_multicontact_mpr 6 -> 4
func_contact_edge_sdf 6 -> 4
Lines at >=8 levels of nesting drop ~38% (381 -> 237); lines at >=11 levels drop ~61% (98 -> 38). File line count: 2491 -> 2482.
func_convex_convex_contact's max depth (14) is unchanged because its outermost wrapif not (PLANE and BOX): <whole body>is a function-level guard, not a loop tail -- flattening it requires return-inside-if support in qd.func, which is a separate work item.Two multi-line conditions (sites at lines 442 and 2045) were negated by hand via De Morgan, preserving their multi-line layout. All other sites are single-line condition flips. File was syntax-checked after every mutation; semantics are preserved by construction for the
continuesubstitution and by inspection for the negations.Description
Related Issue
Resolves Genesis-Embodied-AI/Genesis#
Motivation and Context
How Has This Been / Can This Be Tested?
Screenshots (if appropriate):
Checklist:
Submitting Code Changessection of CONTRIBUTING document.