fix(tf): initialize NVNMD map outputs - #5847
Conversation
Correct the MapFltNvnmd four-input contract, validate its table metadata, and zero outputs that fall outside every mapping interval. Cover deterministic allocator reuse, gradient behavior, and malformed table-gradient shapes. Coding-Agent: Codex Codex-Version: codex-cli 0.144.4 Model: gpt-5.6-sol Reasoning-Effort: xhigh
|
Warning Review limit reached
Next review available in: 15 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesMapFltNvnmd runtime behavior
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
source/op/tf/map_flt_nvnmd.cc (1)
117-134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInterval metadata is parsed twice (validation loop here, then again in the main
ssloop below).Correctness looks solid —
n0>=0 && n1<=shT.dim_size(0)prevents the OOB table read, anddx>0/n1>n0avoid divide-by-zero and negative clamp indices. As an optional cleanup, consider extracting a small helper (e.g. astruct Interval { x0,x1,dx; int n0,n1; }parsed once perss) so the same five floats aren't decoded frominfoin two separate loops.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/op/tf/map_flt_nvnmd.cc` around lines 117 - 134, Extract the repeated interval metadata decoding into a small reusable representation, such as an Interval struct, and parse each five-value entry from info once per ss. Reuse the parsed x0, x1, dx, n0, and n1 values in both the validation loop and the main ss processing loop, preserving the existing validation and mapping behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@source/op/tf/map_flt_nvnmd.cc`:
- Around line 117-134: Extract the repeated interval metadata decoding into a
small reusable representation, such as an Interval struct, and parse each
five-value entry from info once per ss. Reuse the parsed x0, x1, dx, n0, and n1
values in both the validation loop and the main ss processing loop, preserving
the existing validation and mapping behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6ef6e5ae-b840-4092-ba06-c22e30fbfc19
📒 Files selected for processing (2)
source/op/tf/map_flt_nvnmd.ccsource/tests/tf/test_nvnmd_op.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5847 +/- ##
==========================================
- Coverage 79.21% 78.97% -0.24%
==========================================
Files 1069 1069
Lines 124070 124088 +18
Branches 4522 4531 +9
==========================================
- Hits 98278 97996 -282
- Misses 24171 24472 +301
+ Partials 1621 1620 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
Good catch on the real defect here — the stale DCHECK_EQ(3, ...) never fired in release while the op reads input(3), and the unwritten output cells genuinely returned allocator contents. The mechanics of the fix check out: the 4-input requirement matches REGISTER_OP, every rank check precedes the corresponding dim_size() read, the std::fill covers exactly the N*D*M index space the loop writes, and the interval invariants bound the table index tightly enough that no finite x can overflow it.
Two things to resolve before this merges — one semantic, one coverage. Details inline.
Values outside every configured interval mapped to zero, which is a legitimate embedding output rather than a sentinel and diverges from MapTable.mapping() in mapt.py. Select the first containing interval per element and clamp to its nearest edge instead, keeping y continuous and matching the numpy twin. NvnmdConfig.get_s_range() only warns when the s range exceeds the s2g table, so this path is reachable. Cover every OP_REQUIRES site, including each sub-condition of the per-interval table_info check.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
source/op/tf/map_flt_nvnmd.cc (1)
168-168: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment says "first interval" but the clamp uses the selected interval.
id = 0yieldsidx = N0of intervalss(the fall-through one is the last interval, not the first). Suggest "left edge of the selected interval" to match the code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/op/tf/map_flt_nvnmd.cc` at line 168, Update the comment above the below-table evaluation in the interval-selection logic to describe evaluating at the left edge of the selected interval, replacing the inaccurate “first interval” wording; leave the implementation unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@source/op/tf/map_flt_nvnmd.cc`:
- Around line 164-179: Update the interval-selection logic preceding the shown
clamp so NaN xi values are routed into the lower-bound path rather than reaching
int(id). Invert or otherwise adjust the first interval validation condition
while preserving existing finite-value behavior, ensuring id is always a valid
table interval before indexing table.
---
Nitpick comments:
In `@source/op/tf/map_flt_nvnmd.cc`:
- Line 168: Update the comment above the below-table evaluation in the
interval-selection logic to describe evaluating at the left edge of the selected
interval, replacing the inaccurate “first interval” wording; leave the
implementation unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 838ce316-ce89-4fef-8562-71412cc68dfd
📒 Files selected for processing (2)
source/op/tf/map_flt_nvnmd.ccsource/tests/tf/test_nvnmd_op.py
Address the outstanding requested-change review comments. Coding-Agent: Codex Codex-Version: codex-cli 0.144.6 Model: gpt-5.6-sol Reasoning-Effort: xhigh
Resolve the test-file append conflict by retaining both the NVNMD map regressions and the new master neighbor-list tests. Coding-Agent: Codex Codex-Version: codex-cli 0.144.6 Model: gpt-5.6-sol Reasoning-Effort: xhigh
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
Both points are resolved, and I checked them against 1f117d224 rather than the replies.
Edge-clamping was the right call. The restructured loop picks the first containing interval and lets a non-matching value fall through to the widest one, so y stays continuous across the whole real line and the closest neighbor pairs no longer lose their force contribution. Above the table it reproduces mapt.py's idx_k = N1 - 1 with dxx_k = dx; below it, the left edge is the sensible continuous analogue given numpy wraps on a negative index there. idx stays inside N0 .. N1 - 1, and since the per-interval guard enforces N1 <= rows, the table read is bounded. Good that the comment now records the get_s_range() reachability argument rather than just the mechanics.
The guard coverage is exactly what I asked for: seven subTests for the interval field conditions, dedicated tests for the rank/width/length checks, and test_accepts_the_minimal_valid_inputs as a positive control over the shared fixture so each rejection isolates one cause. The positive control was not something I raised and it is the right way to build this.
On why the original defect was invisible: the old interval-outer pass only wrote elements that matched an interval, and no test fed an out-of-range x -- but even one that did would probably have passed, since a fresh allocation reads back as zero. test_out_of_range_values_clamp_to_the_table_edge handles both halves by warming the buffer with warm_x first, so a skipped write cannot masquerade as a correct zero. That test fails on the pre-PR code and also on the intermediate zero-fill version, which is what makes it a real regression test rather than a restatement.
The NaN commit is a genuine improvement I had not asked for. Writing the test as !(id >= 0) is the correct idiom -- every comparison against NaN is false, so the negation routes it to a valid edge instead of reaching the undefined int(id) conversion -- and the comment explains precisely that.
Unrelated to this diff: docs/readthedocs.org:deepmd is failing, but the PR touches only the op and its test, so that is infrastructure rather than anything to fix here.
18cbfd8
3 participants
Summary
MapFltNvnmd's stale three-input check to its registered four-input contractWhy existing tests missed this
test_nvnmd_op.pydirectly covered the other NVNMD arithmetic primitives but notMapFltNvnmd. Normal descriptor and mapping-generation callers keep values inside generated table ranges, release builds compile out the staleDCHECK, and freshly allocated output pages often happen to contain zeros. Together those conditions hid both the invalid debug contract and skipped output writes.The regression warms a large same-shaped output with nonzero values before evaluating out-of-range inputs, making allocator reuse expose the old behavior reliably instead of depending on fresh memory contents.
Validation
cmake --build source/build --target deepmd_op -j2TestOpMapFltNvnmd(2 passed; inherited TensorFlow session helper skipped)ruff format .ruff check .--Werrorgit diff --checkFixes #5655
Coding agent: Codex
Codex version: codex-cli 0.144.4
Model: gpt-5.6-sol
Reasoning effort: xhigh
Summary by CodeRabbit