Skip to content

fix(tf): preserve non-PBC ASE neighbor semantics#5859

Open
njzjz-bot wants to merge 2 commits into
deepmodeling:masterfrom
njzjz-bot:fix/tf1-ase-non-pbc-5668
Open

fix(tf): preserve non-PBC ASE neighbor semantics#5859
njzjz-bot wants to merge 2 commits into
deepmodeling:masterfrom
njzjz-bot:fix/tf1-ase-non-pbc-5668

Conversation

@njzjz-bot

@njzjz-bot njzjz-bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Closes #5668.

Summary

  • preserve the original cells=None decision separately from the identity box required by TensorFlow placeholders
  • pass a missing cell to ASE for open systems and make both TF1 neighbor-list builders handle that documented input without creating periodic ghosts
  • correct the builders return annotations and document their open-boundary behavior
  • add collected regressions for DeepPotential and for DeepTensor eval/eval_full

Why existing tests missed this

  • the original external ASE neighbor-list tests were designed around periodic systems and never exercised cells=None
  • the parameterized TestDeepPot symbol is replaced by object, so TestDeepPotNeighborList did not inherit the base evaluation tests; collection contained only two locally declared skipped tests
  • although the DeepPotential testcase YAML contains a box: null result, it therefore never ran through the ASE-backed evaluator
  • DeepTensor neighbor-list tests inherited PBC fixtures and had no native-versus-ASE open-boundary comparison

Validation

  • focused DeepPotential no-PBC regression: passed
  • focused DeepTensor eval/eval_full no-PBC regression: passed
  • full TestDeepDipoleNewPBCNeighborList class: 10 passed, 2 skipped
  • TF1 DeepPotential periodic native-versus-ASE parity smoke: passed
  • pytest collect-only confirms both new regressions are collected
  • ruff format .
  • ruff check .
  • git diff --check

Coding agent: Codex
Codex version: codex-cli 0.144.4
Model: gpt-5.6-sol
Reasoning effort: xhigh

Summary by CodeRabbit

  • Bug Fixes

    • Fixed neighbor-list construction for open-boundary (no PBC) systems to preserve correct None/boundary semantics.
    • Prevented internal placeholder/identity cell inputs from affecting periodicity detection during neighbor and ghost-atom generation.
    • Ensured boundary semantics stay consistent across energy, tensor, and dipole evaluation paths.
  • Tests

    • Added new open-boundary neighbor-list verification against reference values.
    • Added a dipole test confirming neighbor-list-based outputs match native neighbor-list behavior.

@dosubot dosubot Bot added the bug label Jul 17, 2026
@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@njzjz, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 24 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4ca23923-be01-44f4-a0bb-6ebb137268e1

📥 Commits

Reviewing files that changed from the base of the PR and between f75ab0b and 8d56cb1.

📒 Files selected for processing (1)
  • source/tests/tf/test_deepdipole.py
📝 Walkthrough

Walkthrough

The change preserves open-boundary semantics for TensorFlow inference with ASE neighbor lists, separates placeholder cells from boundary cells, updates neighbor-list annotations and documentation, and adds regression tests for DeepEval and DeepDipole.

Changes

Non-PBC neighbor-list inference

Layer / File(s) Summary
DeepEval boundary handling
deepmd/tf/infer/deep_eval.py
DeepEval and DeepEvalOld accept None as an open-boundary cell, use zero cells for neighbor-list construction, preserve periodicity separately, and update return annotations.
DeepTensor boundary handling
deepmd/tf/infer/deep_tensor.py
eval and eval_full pass cells to neighbor-list construction only when periodic boundaries are enabled.
Open-boundary regression coverage
source/tests/infer/test_models.py, source/tests/tf/test_deepdipole.py
Tests validate non-PBC ASE neighbor-list outputs against reference values and native DeepDipole evaluation.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested labels: Python

Suggested reviewers: njzjz

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main fix: preserving non-PBC ASE neighbor-list semantics in TF inference.
Linked Issues check ✅ Passed The changes address #5668 by preserving None-cell semantics in DeepEval and DeepTensor and adding non-PBC regression tests.
Out of Scope Changes check ✅ Passed The PR stays focused on the reported neighbor-list bug, with only related annotation, doc, and regression-test updates.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Keep the original open-boundary decision separate from the identity box required by TensorFlow feeds, and let both ASE builders handle a missing cell without creating periodic ghosts.

Add collected DeepPotential and DeepTensor regressions for non-periodic external neighbor-list inference.

Coding-Agent: Codex
Codex-Version: codex-cli 0.144.4
Model: gpt-5.6-sol
Reasoning-Effort: xhigh
@njzjz-bot
njzjz-bot force-pushed the fix/tf1-ase-non-pbc-5668 branch from 9212126 to f75ab0b Compare July 17, 2026 01:04
Comment thread source/tests/tf/test_deepdipole.py Fixed
@codecov

codecov Bot commented Jul 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.90%. Comparing base (6c3b985) to head (f75ab0b).

Files with missing lines Patch % Lines
deepmd/tf/infer/deep_eval.py 66.66% 1 Missing ⚠️
deepmd/tf/infer/deep_tensor.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5859      +/-   ##
==========================================
- Coverage   78.58%   76.90%   -1.68%     
==========================================
  Files        1050     1050              
  Lines      120637   120615      -22     
  Branches     4356     4347       -9     
==========================================
- Hits        94801    92758    -2043     
- Misses      24278    26509    +2231     
+ Partials     1558     1348     -210     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@njzjz

njzjz commented Jul 18, 2026

Copy link
Copy Markdown
Member

Possible reviewers based on changed lines, exact file history, and exact-file review history:

  • @wanghan-iapcm — 4 commits on changed files; 17 reviews on exact changed files (deepmd/tf/infer/deep_eval.py, source/tests/infer/test_models.py).
  • @iProzd — 2 commits on changed files (deepmd/tf/infer/deep_eval.py, source/tests/infer/test_models.py).

No review request was made automatically.

Coding agent: Codex
Codex version: codex-cli 0.144.4
Model: gpt-5.6-sol
Reasoning effort: xhigh

@njzjz
njzjz requested review from iProzd and wanghan-iapcm and removed request for iProzd and wanghan-iapcm July 18, 2026 07:26
Coding-Agent: Codex
Codex-Version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning-Effort: xhigh
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Code scan] Preserve non-PBC semantics for TF1 ASE neighbor-list inference

3 participants