deepmodeling / deepmodeling/deepmd-kit
[BUG] neighbor_list behaves three different ways across backends (implemented / silently ignored / raising)
- Dominant language
- Python
- Stars
- 2k
- Forks
- 649
- Avg merge
- 6d 18h
- Merged PRs (30d)
- 15
Description
`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.
Contributor guide
Assessment
This issue has not been assessed yet.