Skip to content

Commit cc81d7b

Browse files
committed
fix(charge-state): validate conditions from training data and malformed archives
The training pipeline registered `charge_spin` as a two-value data requirement and never checked the values, so a dataset carrying a fractional condition reached the unguarded embedding gathers and was truncated onto a neighbouring table row. Validation now runs in `normalize_batch`, the one place every backend and both the standard and LMDB data systems read a batch through, and ahead of tensor conversion so no device synchronization is needed. The rule is evaluated over the whole batch at once, so the training loop pays no per-frame cost, and the single-state and batch entry points share one implementation. On the C++ side the range list from the archive is now required to hold one range per value: a shorter or longer list would have checked one value against another value's table. An empty range is rejected for the same reason. Also registers DPA4C in the family's deepspin rejection list, which is a tuple membership test that its name did not match, and gives it the escape hatch the base contract requires of any descriptor overriding `uses_graph_lower`. DPA4C refuses rather than no-ops there: it carries every neighbour within the cutoff and reports an unreachable `sel`, so a dense lower sized from that capacity is unallocatable rather than merely slower. Two DPA4 fixtures named `[0.5, -0.5]`, which truncates to charge 0 and multiplicity 0; they now name a state the tables can address.
1 parent ecad7fa commit cc81d7b

14 files changed

Lines changed: 356 additions & 29 deletions

File tree

deepmd/dpmodel/descriptor/dpa4c.py

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2021,6 +2021,28 @@ def uses_graph_lower(self) -> bool:
20212021
"""Return whether graph-native lowering is supported."""
20222022
return True
20232023

2024+
def disable_graph_lower(self) -> None:
2025+
"""Reject the dense lower, which this descriptor cannot serve.
2026+
2027+
The base contract asks a descriptor that overrides
2028+
:meth:`uses_graph_lower` to provide the matching escape hatch. DPA4C
2029+
has no dense form to escape to: it carries every neighbour within the
2030+
cutoff and reports an unreachable :meth:`get_sel`, so a dense lower
2031+
sized from that capacity is not a slower route but an unallocatable
2032+
one. Refusing is therefore the honest answer, where a silent no-op
2033+
would leave the caller believing the route had been taken.
2034+
2035+
Raises
2036+
------
2037+
NotImplementedError
2038+
Always, because the graph lower is the only form DPA4C has.
2039+
"""
2040+
raise NotImplementedError(
2041+
"DPA4C is graph-native and has no dense lower to fall back on; "
2042+
"it carries every neighbor within the cutoff rather than a "
2043+
"bounded `sel`."
2044+
)
2045+
20242046
def graph_edge_dtype(self) -> str:
20252047
"""Return the edge-geometry dtype accepted by graph deployment.
20262048

deepmd/dpmodel/model/model.py

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,14 @@
5050
)
5151
get_zbl_model = _model_factory.get_zbl_model
5252

53-
_DPA4_SEZM_DESCRIPTOR_TYPES = ("dpa4", "DPA4", "sezm", "SeZM")
53+
_DPA4_SEZM_DESCRIPTOR_TYPES = (
54+
"dpa4",
55+
"DPA4",
56+
"dpa4c",
57+
"DPA4C",
58+
"sezm",
59+
"SeZM",
60+
)
5461

5562

5663
def get_standard_model(data: dict) -> BaseModel:

deepmd/dpmodel/utils/batch.py

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,10 @@
77

88
import numpy as np
99

10+
from deepmd.utils.charge_state import (
11+
validate_charge_states,
12+
)
13+
1014
# Keys that are metadata / not needed by models or loss functions.
1115
_DROP_KEYS = {"default_mesh", "sid", "fid"}
1216

@@ -36,6 +40,11 @@ def normalize_batch(batch: dict[str, Any]) -> dict[str, Any]:
3640
* ``find_*`` flags are converted to ``np.bool_``.
3741
* Metadata keys (``default_mesh``, ``sid``, ``fid``) are dropped.
3842
43+
Every backend reads its batches through here, so this is also where a
44+
frame condition is checked against the charge and multiplicity tables it
45+
indexes. Doing it on the numpy batch keeps the check off the compiled
46+
forward, where an out-of-range row would reach an unguarded gather.
47+
3948
Parameters
4049
----------
4150
batch : dict[str, Any]
@@ -65,6 +74,9 @@ def normalize_batch(batch: dict[str, Any]) -> dict[str, Any]:
6574
else:
6675
out[key] = val
6776

77+
if out.get("charge_spin") is not None and bool(out.get("find_charge_spin", True)):
78+
validate_charge_states(out["charge_spin"])
79+
6880
return out
6981

7082

deepmd/pt_expt/infer/charge_state.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,8 @@ def charge_states(charge_spin: Any, width: int) -> np.ndarray:
5656
If the request does not hold at least one whole state, or names a
5757
state that no embedding table row answers.
5858
"""
59+
if width <= 0:
60+
raise ValueError(f"a charge state must be at least one value wide, got {width}")
5961
values = np.asarray(charge_spin, dtype=np.float64).reshape(-1)
6062
if values.size == 0 or values.size % width:
6163
raise ValueError(

deepmd/utils/charge_state.py

Lines changed: 71 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,11 @@
1111
Neither the gather nor the compiled kernel bounds-checks the row index, so a
1212
value outside a table reads past it and a fractional value is silently
1313
truncated onto a neighbouring row. Every host-side boundary that accepts a
14-
condition therefore passes it through :func:`validate_charge_state` first. The
15-
per-forward path is deliberately not guarded: its values come from the data
16-
pipeline, which owns their validity exactly as it owns the validity of an atom
17-
type.
14+
condition therefore checks it here first: a configured default through
15+
:func:`validate_charge_state`, and a batch of frames read from the training
16+
data through :func:`validate_charge_states`. Both share one rule, evaluated
17+
over the whole batch at once so the training loop pays no per-frame cost, and
18+
both run before tensor conversion so no device synchronization is needed.
1819
"""
1920

2021
from __future__ import (
@@ -53,6 +54,50 @@
5354
CHARGE_STATE_WIDTH = len(CHARGE_STATE_FIELDS)
5455

5556

57+
def _as_states(charge_spin: Any) -> np.ndarray:
58+
"""Read a request as ``(n, CHARGE_STATE_WIDTH)`` states."""
59+
values = np.asarray(charge_spin, dtype=np.float64).reshape(-1)
60+
if values.size == 0 or values.size % CHARGE_STATE_WIDTH:
61+
raise ValueError(
62+
f"A charge state must be a `[charge, multiplicity]` pair, got "
63+
f"{values.size} values"
64+
)
65+
return values.reshape(-1, CHARGE_STATE_WIDTH)
66+
67+
68+
def _check_states(states: np.ndarray) -> None:
69+
"""Reject any state that addresses no row of the embedding tables.
70+
71+
The whole batch is tested column by column with array operations, so the
72+
cost does not grow with the number of frames. A non-finite value is
73+
reported as the integrality failure rather than reaching the range test,
74+
whose message would have to render it.
75+
76+
Parameters
77+
----------
78+
states : np.ndarray
79+
Charge states with shape (n, CHARGE_STATE_WIDTH).
80+
81+
Raises
82+
------
83+
ValueError
84+
If any value is not an integer inside its table's row range.
85+
"""
86+
integral = np.isfinite(states) & (states == np.floor(states))
87+
for column, (name, (low, high)) in enumerate(
88+
zip(CHARGE_STATE_FIELDS, CHARGE_STATE_TABLE_RANGES, strict=True)
89+
):
90+
values = states[:, column]
91+
offending = values[~integral[:, column]]
92+
if offending.size:
93+
raise ValueError(f"The {name} must be an integer, got {offending[0]}")
94+
outside = values[(values < low) | (values >= high)]
95+
if outside.size:
96+
raise ValueError(
97+
f"The {name} must lie in [{low}, {high}), got {outside[0]:.0f}"
98+
)
99+
100+
56101
def validate_charge_state(charge_spin: Any) -> list[float]:
57102
"""Check that a frame condition addresses a row of each embedding table.
58103
@@ -72,22 +117,28 @@ def validate_charge_state(charge_spin: Any) -> list[float]:
72117
If the pair does not hold exactly two integral values within the
73118
representable ranges.
74119
"""
75-
values = [float(value) for value in np.reshape(np.asarray(charge_spin), (-1,))]
76-
if len(values) != CHARGE_STATE_WIDTH:
120+
states = _as_states(charge_spin)
121+
if states.shape[0] != 1:
77122
raise ValueError(
78123
f"A charge state must be a `[charge, multiplicity]` pair, got "
79-
f"{len(values)} values"
124+
f"{states.size} values"
80125
)
81-
for value, name, (low, high) in zip(
82-
values,
83-
CHARGE_STATE_FIELDS,
84-
CHARGE_STATE_TABLE_RANGES,
85-
strict=True,
86-
):
87-
if not np.isfinite(value) or value != int(value):
88-
raise ValueError(f"The {name} must be an integer, got {value}")
89-
if not low <= value < high:
90-
raise ValueError(
91-
f"The {name} must lie in [{low}, {high}), got {int(value)}"
92-
)
93-
return values
126+
_check_states(states)
127+
return states[0].tolist()
128+
129+
130+
def validate_charge_states(charge_spin: Any) -> None:
131+
"""Check every frame condition of a batch against the embedding tables.
132+
133+
Parameters
134+
----------
135+
charge_spin
136+
One ``[charge, multiplicity]`` pair per frame, in any shape holding a
137+
whole number of pairs.
138+
139+
Raises
140+
------
141+
ValueError
142+
If any frame names a state that no table row answers.
143+
"""
144+
_check_states(_as_states(charge_spin))

source/api_cc/src/DeepPotPTExpt.cc

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,7 @@ void DeepPotPTExpt::init(const std::string& model,
224224
}
225225
}
226226
default_chg_spin_ = read_default_chg_spin(metadata, dchgspin);
227-
chg_spin_table_ranges_ = read_chg_spin_table_ranges(metadata);
227+
chg_spin_table_ranges_ = read_chg_spin_table_ranges(metadata, dchgspin);
228228
check_charge_spin_domain(default_chg_spin_, chg_spin_table_ranges_);
229229

230230
if (metadata.obj_val.count("do_atomic_virial")) {
@@ -397,7 +397,8 @@ void DeepPotPTExpt::init(const std::string& model,
397397
// The constants were frozen against the archive's own charge state, so
398398
// that is the state in force until ``set_charge_spin`` installs another.
399399
default_chg_spin_ = read_default_chg_spin(metadata, settable_chgspin);
400-
chg_spin_table_ranges_ = read_chg_spin_table_ranges(metadata);
400+
chg_spin_table_ranges_ =
401+
read_chg_spin_table_ranges(metadata, settable_chgspin);
401402
check_charge_spin_domain(default_chg_spin_, chg_spin_table_ranges_);
402403
}
403404

source/api_cc/src/DeepSpinPTExpt.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,7 @@ void DeepSpinPTExpt::init(const std::string& model,
148148
}
149149
}
150150
default_chg_spin_ = read_default_chg_spin(metadata, dchgspin);
151-
chg_spin_table_ranges_ = read_chg_spin_table_ranges(metadata);
151+
chg_spin_table_ranges_ = read_chg_spin_table_ranges(metadata, dchgspin);
152152
check_charge_spin_domain(default_chg_spin_, chg_spin_table_ranges_);
153153

154154
if (metadata.obj_val.count("do_atomic_virial")) {

source/api_cc/src/NativeSpinPTExpt.cc

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -268,7 +268,7 @@ void NativeSpinPTExpt::init(const std::string& model,
268268
// compressed model, whose lower has no conditioning input at all.
269269
settable_chgspin = dchgspin;
270270
default_chg_spin_ = read_default_chg_spin(metadata, dchgspin);
271-
chg_spin_table_ranges_ = read_chg_spin_table_ranges(metadata);
271+
chg_spin_table_ranges_ = read_chg_spin_table_ranges(metadata, dchgspin);
272272
check_charge_spin_domain(default_chg_spin_, chg_spin_table_ranges_);
273273
has_default_fparam_ = metadata.obj_val.count("has_default_fparam") &&
274274
metadata["has_default_fparam"].as_bool();
@@ -395,7 +395,8 @@ void NativeSpinPTExpt::init(const std::string& model,
395395
// The constants were frozen against the archive's own charge state, so
396396
// that is the state in force until ``set_charge_spin`` installs another.
397397
default_chg_spin_ = read_default_chg_spin(metadata, settable_chgspin);
398-
chg_spin_table_ranges_ = read_chg_spin_table_ranges(metadata);
398+
chg_spin_table_ranges_ =
399+
read_chg_spin_table_ranges(metadata, settable_chgspin);
399400
check_charge_spin_domain(default_chg_spin_, chg_spin_table_ranges_);
400401
}
401402

source/api_cc/src/commonPTExpt.h

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -293,12 +293,17 @@ inline std::vector<double> read_default_chg_spin(const JsonValue& metadata,
293293
* table. An archive frozen before the ranges were recorded names none, and
294294
* the boundary then checks only the width, as it did before.
295295
*
296+
* The ranges are indexed by column, so one per value of a charge state is the
297+
* only count that lines the domain check up with the values it guards. A
298+
* shorter or longer list would check a value against another value's table.
299+
*
296300
* @param[in] metadata Parsed archive metadata.
301+
* @param[in] dim_chg_spin Width of a charge state the model accepts.
297302
* @return One ``{low, high}`` pair per value, empty when the archive names
298303
* no ranges.
299304
**/
300305
inline std::vector<std::pair<double, double>> read_chg_spin_table_ranges(
301-
const JsonValue& metadata) {
306+
const JsonValue& metadata, const int dim_chg_spin) {
302307
std::vector<std::pair<double, double>> ranges;
303308
if (!metadata.obj_val.count("chg_spin_table_ranges") ||
304309
metadata["chg_spin_table_ranges"].type == JsonValue::Null) {
@@ -310,8 +315,19 @@ inline std::vector<std::pair<double, double>> read_chg_spin_table_ranges(
310315
throw deepmd::deepmd_exception(
311316
"chg_spin_table_ranges must hold a [low, high) pair per value.");
312317
}
318+
if (pair[0].as_double() >= pair[1].as_double()) {
319+
throw deepmd::deepmd_exception(
320+
"chg_spin_table_ranges names an empty [low, high) range, which no "
321+
"charge state can address.");
322+
}
313323
ranges.emplace_back(pair[0].as_double(), pair[1].as_double());
314324
}
325+
if (static_cast<int>(ranges.size()) != dim_chg_spin) {
326+
throw deepmd::deepmd_exception(
327+
"chg_spin_table_ranges holds " + std::to_string(ranges.size()) +
328+
" ranges but the model accepts charge states of width " +
329+
std::to_string(dim_chg_spin) + ".");
330+
}
315331
return ranges;
316332
}
317333

source/tests/common/dpmodel/test_descriptor_dpa4c.py

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1798,3 +1798,50 @@ def test_the_calibration_samples_the_corpus_charge_states() -> None:
17981798
[{key: value for key, value in corpus[0].items() if key != "charge_spin"}]
17991799
)
18001800
assert not np.allclose(descriptor.stddev, default_state.stddev)
1801+
1802+
1803+
def test_dense_lower_escape_hatch_is_refused() -> None:
1804+
"""DPA4C overrides ``uses_graph_lower`` and owes the paired hatch.
1805+
1806+
The base contract requires the postcondition ``uses_graph_lower() is
1807+
False`` after the hatch. DPA4C has no dense form to reach: it carries
1808+
every neighbour within the cutoff and reports an unreachable ``get_sel``,
1809+
so it refuses instead of inheriting a no-op that reports success while
1810+
leaving the graph lower in place.
1811+
"""
1812+
descriptor = DescrptDPA4C(
1813+
rcut=4.0, ntypes=2, channels=8, lmax=2, n_radial=4, seed=1
1814+
)
1815+
assert descriptor.uses_graph_lower()
1816+
with pytest.raises(NotImplementedError, match="no dense lower"):
1817+
descriptor.disable_graph_lower()
1818+
assert descriptor.uses_graph_lower()
1819+
1820+
1821+
def test_virtual_atom_spin_scheme_is_refused() -> None:
1822+
"""DPA4C is a DPA4-family descriptor, so deepspin must be rejected.
1823+
1824+
``SpinModel`` pulls the dense escape hatch its descriptors do not have,
1825+
and the family's constraint lives in the model factory, so a new member
1826+
has to be registered there rather than rely on a downstream failure.
1827+
"""
1828+
from deepmd.dpmodel.model.model import (
1829+
get_spin_model,
1830+
)
1831+
1832+
with pytest.raises(NotImplementedError, match="deepspin"):
1833+
get_spin_model(
1834+
{
1835+
"type_map": ["Ni", "O"],
1836+
"spin": {"use_spin": [True, False]},
1837+
"descriptor": {
1838+
"type": "dpa4c",
1839+
"rcut": 4.0,
1840+
"channels": 8,
1841+
"lmax": 2,
1842+
"n_radial": 4,
1843+
"seed": 1,
1844+
},
1845+
"fitting_net": {"neuron": [8, 8], "seed": 1},
1846+
}
1847+
)

0 commit comments

Comments
 (0)