[pull] master from deepmodeling:master - #311
Merged
Merged
Conversation
…odels (#5966) Close #5927 — resolved **as designed**, with documentation and semantic pin tests instead of a behavior change. ## The definitions (verified against the code) The model energy decomposes as `E = E_model + E_bias`, where `E_model` is everything the model computes (learned network, analytical ZBL term, or a `linear_ener` combination) and `E_bias` is the per-type output bias. - **`set-by-statistic`** assigns `E_bias` directly: user values, or the per-type least-squares statistic of the **raw labels**. It never calls a model forward (`compute_output_stats` with `model_forward=None`, stored with `add=False`) — it is independent of `E_model` **by definition**, ignoring a trained network and the analytical ZBL term alike. - **`change-by-statistic`** fits the residual of the labels against the **complete model prediction** (the bridged predictor since #5910) and adds the delta to the existing bias. Under these definitions the "double count" described in the issue is not a bridging bug: `set` uniformly ignores all of `E_model` for every model kind. A bridged model after `set` carries no compensation for the mean ZBL contribution — exactly as a trained plain model after `set` carries no compensation for its network output. The two modes "disagree" because they are defined to answer different questions; a self-consistent calibration is `change`'s job. Verified conformance of the `linear_ener` composition path in both backends: children compute no output statistics (`compute_or_load_out_stat=False`); the composition level performs one `set` fit on the raw labels. Also verified: the ZBL term contributes exactly zero for isolated atoms, so its own statistics are trivially `bias = 0`. ## Changes - `doc/model/change-bias.md`: precise definitions of the two modes, and the guidance that calibrating a bridged (or any nonzero-`E_model`) model self-consistently requires `change-by-statistic`. - `doc/model/dpa4.md`: note in the ZBL section. - Semantic pin tests (pt `SeZMModel` + dpmodel `LinearEnergyAtomicModel` composition): `set-by-statistic` equals the raw-label least-squares fit exactly — guarding against a future "fix" that would subtract the analytical term and silently create a third, model-dependent mode. ## Known limitations - No behavior change anywhere; the pin tests cover `model.change_out_bias` (the `dp change-bias --mode set` and finetune routes) and the dpmodel composition out-stat; the pt training-init chain was verified in-session and funnels into the same pinned branch. - The spin variants share the machinery but have no dedicated pin. - Fact worth knowing when reading the docs: `InnerPotential` adds the full ZBL over the whole cutoff (not only below `bridging_r_outer`), so the label-side ZBL at equilibrium geometries is small but not strictly zero; the docs state the offset plainly. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Clarified how `set` and `change` statistic modes handle model energy, analytical contributions, labels, residuals, and existing bias. * Documented the interaction between output-bias calibration and ZBL bridging, including guidance for self-consistent calibration. * **Tests** * Added regression coverage confirming that `set-by-statistic` calibration uses raw energy labels independently of learned and ZBL model contributions. * Verified calibration correctly replaces seeded bias and remains consistent when repeated. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Han Wang <wang_han@iapcm.ac.cn>
Fixes #5627 ## Summary - allocate the host copies of scalar `nlocal` and `nghost` with one element instead of sizing them by `nswap` - add a documented local-copy helper that chooses `gpuMemcpy` only for actual GPU places and uses host `memcpy` for CPU or host-pinned places - use the same place-based dispatch for forward and backward self-swaps - add direct Paddle custom-op tests for `nswap == 0`, CPU self-copy, and the reverse self-swap used by autograd ## Why existing tests missed this The existing Paddle suite did not directly call `border_op`. Model-level tests therefore did not construct the two boundary conditions that matter here: a valid no-swap invocation where the atom-count scalars still need storage, and a self-swap using CPU data from an operator compiled with CUDA support. Normal multi-rank runs also tend to use tensors on the configured accelerator, hiding the mismatch between CUDA-awareness and actual tensor place. The new no-swap test passes empty communication arrays with scalar atom counts, while the self-swap test keeps a real LAMMPS-style pointer-valued send list alive and checks both forward data and backward execution on CPU tensors. In a CUDA-enabled CI build, the historical code would route that CPU pointer through device-to-device `gpuMemcpy`. ## Validation - built the Paddle 3.4 CPU custom op from `source/op/pd/setup.py` - `pytest source/tests/pd/test_border_op.py -q` (2 passed, including backward) - compiled the full `GOOGLE_CUDA + USE_MPI` branch with Paddle, CUDA 12.4, and MPI headers using `mpicxx -fsyntax-only` - `ruff format .` - `ruff check .` - `clang-format --dry-run --Werror source/op/pd/comm.cc` - `git diff --check` The local Paddle wheel is CPU-only, so runtime execution of the CUDA-enabled custom op is left to CUDA CI; the CUDA/MPI branch was still compiled locally, and the new CPU-place self-swap test is designed to run unchanged in that build. Coding agent: Codex Codex version: codex-cli 0.144.4 Model: gpt-5.6-sol Reasoning effort: xhigh <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved tensor copying for border operations across CPU and GPU memory. * Corrected zero-swap handling so outputs remain consistent with inputs. * Ensured self-copy operations use the appropriate device behavior and preserve gradients. * Improved gradient synchronization and handling for GPU-based operations. * **Tests** * Added regression coverage for zero-swap behavior and CPU self-copy execution, including pointer-based transfers, duplicated outputs, and gradient validation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: njzjz-bot <njzjz.bot@gmail.com> Co-authored-by: njzjz-bot <njzjz-bot@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
…es (#5967) ## Summary Promotes duck-typed capability probes (`hasattr`/`getattr(obj, "x", default)`) inventoried in #5897 to concrete-default methods/attributes declared once on the owning base class, so a typo'd or renamed name raises instead of silently degrading to the default. Eight change groups, one commit each (plus one fix-up commit): - Merge `has_default_chg_spin` into `get_default_chg_spin` (derive via `is not None`); drop the charge-spin `hasattr` probe family and declare concrete defaults on the owning bases. - Declare `get_var_name` / `get_task_dim` / `get_intensive` on `make_base_model` with concrete defaults; convert the jax2tf property probe and refresh a stale docstring. - Add a `get_geo_compress()` base accessor and drop consumer `getattr`/`hasattr` probes; pin both the override and default branches with a dedicated compression test. - Add a concrete `reinit_exclude` no-op default on `make_base_fitting`, killing the `dp_atomic_model.py` probe. - Add a `get_pair_exclude_types()` accessor on `BaseAtomicModel` and pin `pair_excl` as a direct-access contract (verified against every construction path, including deserialize and `SpinModel.forward_common_lower`). - Declare `set_davg_zero` / `set_stddev_constant` as class-default stat flags on the descriptor **block** bases (extended to `make_base_descriptor`'s BD base after ratifying that `merge_env_stat`'s real contract is `Union[Descriptor, DescriptorBlock]`, not "blocks only"). - Drop the dead-defensive `has_message_passing` / `has_default_fparam` probes now that both are already declared on the base — call directly. - Declare `tebd_compress` / `type_embd_data` compression slots in the tebd family `__init__`s (dpa1 / se_atten_v2 / se_t_tebd + blocks), including a follow-up fix that defuses a `register_buffer` trap for the DPA2 tebd compression slot. Every promotion ships a dual-branch (default + override) universal-suite assertion; `pair_excl` ships a construction-path assertion instead. Closes #5897 ## Known limitations - pd (Paddle) edits are verified by `py_compile` only — no local Paddle install to run its test suite. - CUDA fused-kernel consumers of `get_geo_compress()` and graph `.pt2` export/AOTI paths are untested on this CPU-only box; GPU validation before merge is advisable. - No dedicated jax `DeepEval` charge-spin unit test (the jax `infer/deep_eval.py` charge-spin path is a one-line delegation, exercised only indirectly). - The native-spin pair-exclusion folding branch of `SpinModel.forward_common_lower` (`pair_exclude_types` set on a native spin model) is untested — no existing test sets it on a spin model. - `DescrptDPA2.get_geo_compress()`'s `True` branch is unpinned: no DPA2 compression test exists at all (pre-existing gap, not introduced here). - The `atom_excl` `getattr` probes (e.g. `deepmd/kernels/cuda/dpa1/canonical.py:44`) are the same duck-typing pattern but were never part of #5897's inventory — candidate follow-up issue, not addressed here. - torch.compile-gated training paths are skipped on this box (torch 2.10/2.11 environment); a handful of `.pt2` graph-freeze tests (`test_dpa4_export.py`, `test_dpa4_zbl_parallel.py`, `test_graph_export_with_comm.py`, `test_zbl_bridging.py`) fail with a torch inductor CPU codegen `AssertionError` (`atomic_add` store on a non-vectorized index) — confirmed pre-existing on a clean `upstream/master` worktree, unrelated to this branch's changes. - Hybrid descriptors' `add_chg_spin_ebd` now derives from `get_dim_chg_spin() > 0` (rather than the previous getattr-probe), so a hybrid descriptor with a chg-spin-capable child now actually conditions on it, where it previously never did. This is a latent-bug fix surfaced by the promotion, not an intentional feature change — it has no dedicated end-to-end test. - The DeepEval facade's `get_var_name()` / `get_intensive()` now return `None` / `False` for live non-property models instead of raising `NotImplementedError`, matching the concrete-default contract on `make_base_model`. Callers that relied on the exception (if any exist outside this repo) would need to switch to checking the return value. - jax's and tf2's `merge_env_stat` multi-task path (multiple descriptors sharing stats across sub-models) has no dedicated unit test; only the single-task path is covered. - pd (Paddle)'s `has_default_chg_spin` → `get_default_chg_spin` interface merge (completing the promotion started elsewhere in the branch) was verified by `python -m py_compile` only — no Paddle install on this box to run pd's test suite. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added APIs for geometric compression status, property metadata, and excluded atom-type pairs. * Added safer atom-type exclusion reconfiguration where supported. * **Improvements** * Standardized charge/spin capability detection across model backends. * Improved serialization of compressed descriptors and optional model capabilities. * **Bug Fixes** * Corrected charge/spin default detection and pair-exclusion handling. * Improved compatibility when loading and exporting models with differing capabilities. * **API Changes** * Replaced the separate charge/spin default check with availability based on returned default values. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Han Wang <wang_han@iapcm.ac.cn> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )