[BUG] neighbor_list behaves three different ways across backends (implemented / silently ignored / raising)

Open
  • Is something wrong?
#5,941 0 comments 0 reactions 0 assignees View on GitHub

Maintainers usually reply within 3 days

Nobody has claimed this yet.

Assessment

Difficulty
4/5
Estimated time
3-5 days
Newbie friendliness
48/100
Issue type
Bug
Clarity
Mostly clear
Activity status
Quiet
Tech stack
python, pytorch, tensorflow
Domain
backend, testing

Research direction

Start with DeepEvalBackend.init and DeepEval.init in deepmd/infer/deep_eval.py, DeepPot in deepmd/infer/deep_pot.py, and TestDeepPotNeighborList in source/tests/infer/test_models.py. Compare neighbor_list handling across the documented backend extensions, then establish and document one consistent contract. Done means the shared docstrings and tests explicitly cover every extension, including future matrix entries.

Written by the indexing model from the issue text.

Description

bug

neighbor_list is a documented keyword on DeepPot / DeepEval, but its behaviour depends on the model file extension in a way nothing surfaces to the caller. After #5858 there are three distinct outcomes:

Backend / extension Behaviour
.pb (tf), .pt2 (pt_expt) Implemented — the ASE object is used to build the neighbor list
.pth (pt), .pd, .dp, .savedmodel (jax) Accepted and silently ignored
.savedmodeltf (tf2) Raises NotImplementedError (added by #5858)

Note deepmd/pt/infer/deep_eval.py belongs in the silently-ignored group despite storing the value: its only read is ase_provided = self.neighbor_list is not None, so the ASE object itself never builds a neighbor list. pd, jax and dpmodel additionally document the parameter while ignoring it.

The silent-no-op cases are the harmful ones — a user passing a custom neighbor list gets the model's built-in one and results that look plausible.

Two concrete gaps

Docstrings promise it unconditionally. DeepEvalBackend.__init__ and DeepEval.__init__ in deepmd/infer/deep_eval.py, and DeepPot in deepmd/infer/deep_pot.py, all say:

The ASE neighbor list class to produce the neighbor list. If None, the neighbor list will be built natively in the model.

So DeepPot("model.savedmodeltf", neighbor_list=nl) is a documented call that now raises, and DeepPot("model.pth", neighbor_list=nl) is a documented call that quietly ignores the argument.

No test would catch the next backend. TestDeepPotNeighborList in source/tests/infer/test_models.py is parameterized to (".pb",) while the base TestDeepPot runs a four-way extension matrix:

https://github.com/deepmodeling/deepmd-kit/blob/69ff61ef1eb3a3ad1a3f42f10d1e5cd1f8d0f0ba/source/tests/infer/test_models.py#L399-L403

A tf2-only "it raises" test cannot prevent recurrence; extending this matrix would.

Suggested direction

Decide the contract once, rather than per backend: either every backend that cannot honour neighbor_list raises NotImplementedError as tf2 now does, or the parameter is documented as advisory. Whichever is chosen, the shared docstrings should state it, and TestDeepPotNeighborList should cover every extension so a new backend inherits the expectation.

Raised while reviewing #5858, which deliberately scoped itself to the tf2 half.

Dominant language
Python
Stars
2.1k
Forks
663
Avg merge
3d 11h
Merged PRs (30d)
25

Getting set up

Open in Codespaces

Starts the project's dev container in your browser, under your own GitHub account.

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from deepmodeling/deepmd-kit

All issues in deepmodeling/deepmd-kit

Similar issues

More Python issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.