feat: add plugin mode for data modifier #4621
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| import numpy as np | ||
|
|
||
| import deepmd.tf.op # noqa: F401 |
Check notice
Code scanning / CodeQL
Unused import Note
| def build_fv_graph(self) -> tf.Tensor: | ||
| """Build the computational graph for the force and virial inference.""" | ||
| with tf.variable_scope("modifier_attr"): | ||
| t_mdl_name = tf.constant(self.model_name, name="mdl_name", dtype=tf.string) |
Check notice
Code scanning / CodeQL
Unused local variable Note
| """Build the computational graph for the force and virial inference.""" | ||
| with tf.variable_scope("modifier_attr"): | ||
| t_mdl_name = tf.constant(self.model_name, name="mdl_name", dtype=tf.string) | ||
| t_modi_type = tf.constant( |
Check notice
Code scanning / CodeQL
Unused local variable Note
| t_modi_type = tf.constant( | ||
| self.modifier_prefix, name="type", dtype=tf.string | ||
| ) | ||
| t_mdl_charge_map = tf.constant( |
Check notice
Code scanning / CodeQL
Unused local variable Note
| name="mdl_charge_map", | ||
| dtype=tf.string, | ||
| ) | ||
| t_sys_charge_map = tf.constant( |
Check notice
Code scanning / CodeQL
Unused local variable Note
| self.t_ef = tf.placeholder(GLOBAL_TF_FLOAT_PRECISION, [None], name="t_ef") | ||
| nf = 10 | ||
| nfxnas = 64 * nf | ||
| nfxna = 192 * nf |
Check warning
Code scanning / CodeQL
Variable defined multiple times Warning
| nfxnas = 64 * nf | ||
| nfxna = 192 * nf | ||
| nf = -1 | ||
| nfxnas = -1 |
Check notice
Code scanning / CodeQL
Unused local variable Note
| corr_av.append(av) | ||
| corr_f = np.concatenate(corr_f, axis=0) | ||
| corr_v = np.concatenate(corr_v, axis=0) | ||
| corr_av = np.concatenate(corr_av, axis=0) |
Check notice
Code scanning / CodeQL
Unused local variable Note
| default_mesh = make_default_mesh(True, False) | ||
|
|
||
| # evaluate | ||
| tensor = [] |
Check notice
Code scanning / CodeQL
Unused local variable Note
| box = data["box"][:get_nframes, :] | ||
| atype = data["type"][:get_nframes, :] | ||
| atype = atype[0] | ||
| nframes = coord.shape[0] |
Check notice
Code scanning / CodeQL
Unused local variable Note
📝 WalkthroughWalkthroughThis pull request introduces new modules and updates to support a more flexible modifier system. A new base class ( Changes
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
deepmd/tf/modifier/base_modifier.py (1)
2-4: Consider optional direct imports for readability.Importing
make_base_modifierfrom a nested path is fine, but if there's frequent usage, an alternative structure or aliasing might improve maintainability and clarity in the codebase.deepmd/dpmodel/modifier/base_modifier.py (2)
23-33: Add more guidance or structure to the abstractserializemethod.Currently, the docstring is minimal, which may cause confusion for implementers about the expected format. Providing a sample return format or clarifying potential fields might improve developer experience.
34-50: Fix docstring mismatch indeserialize.The docstring indicates the return type is
BaseModel, but this file definesBaseModifier. Clarify which entity is actually returned to avoid confusion for implementers.- BaseModel + BaseModifierdeepmd/tf/entrypoints/train.py (1)
279-285: Handle missing or invalid "type" gracefully.If
modi_datalacks a"type"field,modifier_type = modifier_params.pop("type")raises aKeyError. Ensure upstream logic always provides"type"or add a defensive check to avoid unhandled exceptions.modifier_type = modifier_params.pop("type", None) if modifier_type is None: # Provide a default or raise a more user-friendly error raise ValueError("No 'type' specified for the modifier.")source/tests/tf/test_data_modifier.py (1)
100-104: Optional: Ensure consistency by using float literals for default float parameters.
Whileewald_h=1andewald_beta=0.25are acceptable, you might prefer using1.0for consistency with the float annotation.def _test_fv(self) -> None: dcm = DipoleChargeModifier( - model_name=str(tests_path / os.path.join(modifier_datapath, "dipole.pb")), - model_charge_map=[-8], - sys_charge_map=[6, 1], - ewald_h=1, - ewald_beta=0.25, + model_name=str(tests_path / os.path.join(modifier_datapath, "dipole.pb")), + model_charge_map=[-8], + sys_charge_map=[6, 1], + ewald_h=1.0, + ewald_beta=0.25, )deepmd/tf/modifier/dipole_charge.py (5)
48-48: Use the simplerkwargs.get("model_name").
There's no need for the default argument here, as it's checked immediately after.def __new__(cls, *args, **kwargs): - model_file = kwargs.get("model_name", None) + model_file = kwargs.get("model_name") if model_file is None: raise TypeError("Missing required argument: 'model_name'") return super().__new__(cls, model_file)🧰 Tools
🪛 Ruff (0.8.2)
48-48: Use
kwargs.get("model_name")instead ofkwargs.get("model_name", None)Replace
kwargs.get("model_name", None)withkwargs.get("model_name")(SIM910)
136-151: Remove or comment out unused variables to tidy up the code.
These variables (t_mdl_name,t_modi_type,t_mdl_charge_map,t_sys_charge_map,t_ewald_h,t_ewald_b) are assigned but never used.with tf.variable_scope("modifier_attr"): - t_mdl_name = tf.constant(self.model_name, name="mdl_name", dtype=tf.string) - t_modi_type = tf.constant( - self.modifier_prefix, name="type", dtype=tf.string - ) - t_mdl_charge_map = tf.constant( - " ".join([str(ii) for ii in self.model_charge_map]), - name="mdl_charge_map", - dtype=tf.string, - ) - t_sys_charge_map = tf.constant( - " ".join([str(ii) for ii in self.sys_charge_map]), - name="sys_charge_map", - dtype=tf.string, - ) - t_ewald_h = tf.constant(self.ewald_h, name="ewald_h", dtype=tf.float64) - t_ewald_b = tf.constant( - self.ewald_beta, name="ewald_beta", dtype=tf.float64 - ) ...🧰 Tools
🪛 Ruff (0.8.2)
136-136: Local variable
t_mdl_nameis assigned to but never usedRemove assignment to unused variable
t_mdl_name(F841)
137-137: Local variable
t_modi_typeis assigned to but never usedRemove assignment to unused variable
t_modi_type(F841)
140-140: Local variable
t_mdl_charge_mapis assigned to but never usedRemove assignment to unused variable
t_mdl_charge_map(F841)
145-145: Local variable
t_sys_charge_mapis assigned to but never usedRemove assignment to unused variable
t_sys_charge_map(F841)
150-150: Local variable
t_ewald_his assigned to but never usedRemove assignment to unused variable
t_ewald_h(F841)
151-151: Local variable
t_ewald_bis assigned to but never usedRemove assignment to unused variable
t_ewald_b(F841)
159-164: Remove or refactor unused local variables.
The variablesnf,nfxnas, andnfxnaare overwritten and never used in subsequent logic.nf = 10 nfxnas = 64 * nf nfxna = 192 * nf nf = -1 -nfxnas = -1 -nfxna = -1 self.t_box_reshape = tf.reshape(self.t_box, [-1, 9])🧰 Tools
🪛 Ruff (0.8.2)
163-163: Local variable
nfxnasis assigned to but never usedRemove assignment to unused variable
nfxnas(F841)
392-392: Remove the unused local variable.
tensoris assigned but never referenced, so removing it helps keep the code uncluttered.- tensor = [] feed_dict_test = {}
481-481: Drop the redundantnframesassignment.
It's never used and might create confusion with similarly named variables in the same method.def modify_data(self, data: dict, data_sys: DeepmdData) -> None: ... atype = data["type"][:get_nframes, :] - nframes = coord.shape[0] tot_e, tot_f, tot_v = self.eval(coord, box, atype) ...🧰 Tools
🪛 Ruff (0.8.2)
481-481: Local variable
nframesis assigned to but never usedRemove assignment to unused variable
nframes(F841)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
deepmd/dpmodel/modifier/__init__.py(1 hunks)deepmd/dpmodel/modifier/base_modifier.py(1 hunks)deepmd/tf/entrypoints/train.py(3 hunks)deepmd/tf/modifier/__init__.py(1 hunks)deepmd/tf/modifier/base_modifier.py(1 hunks)deepmd/tf/modifier/dipole_charge.py(1 hunks)source/tests/tf/test_data_modifier.py(2 hunks)source/tests/tf/test_data_modifier_shuffle.py(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- deepmd/dpmodel/modifier/init.py
- deepmd/tf/modifier/init.py
- source/tests/tf/test_data_modifier_shuffle.py
🧰 Additional context used
🪛 Ruff (0.8.2)
deepmd/tf/modifier/dipole_charge.py
48-48: Use kwargs.get("model_name") instead of kwargs.get("model_name", None)
Replace kwargs.get("model_name", None) with kwargs.get("model_name")
(SIM910)
136-136: Local variable t_mdl_name is assigned to but never used
Remove assignment to unused variable t_mdl_name
(F841)
137-137: Local variable t_modi_type is assigned to but never used
Remove assignment to unused variable t_modi_type
(F841)
140-140: Local variable t_mdl_charge_map is assigned to but never used
Remove assignment to unused variable t_mdl_charge_map
(F841)
145-145: Local variable t_sys_charge_map is assigned to but never used
Remove assignment to unused variable t_sys_charge_map
(F841)
150-150: Local variable t_ewald_h is assigned to but never used
Remove assignment to unused variable t_ewald_h
(F841)
151-151: Local variable t_ewald_b is assigned to but never used
Remove assignment to unused variable t_ewald_b
(F841)
163-163: Local variable nfxnas is assigned to but never used
Remove assignment to unused variable nfxnas
(F841)
393-393: Local variable tensor is assigned to but never used
Remove assignment to unused variable tensor
(F841)
481-481: Local variable nframes is assigned to but never used
Remove assignment to unused variable nframes
(F841)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Test Python (5, 3.9)
- GitHub Check: Test Python (1, 3.12)
- GitHub Check: Test Python (2, 3.9)
- GitHub Check: Build C library (2.14, >=2.5.0rc0,<2.15, libdeepmd_c_cu11.tar.gz)
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build wheels for cp311-macosx_x86_64
- GitHub Check: Build wheels for cp311-macosx_arm64
- GitHub Check: Build wheels for cp311-win_amd64
- GitHub Check: Build wheels for cp310-manylinux_aarch64
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build C++ (clang, clang)
- GitHub Check: Test C++ (false)
- GitHub Check: Analyze (python)
- GitHub Check: Test C++ (true)
- GitHub Check: Build C++ (cuda, cuda)
- GitHub Check: Build C++ (rocm, rocm)
- GitHub Check: Build C++ (cpu, cpu)
- GitHub Check: Build C++ (cuda120, cuda)
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (3)
deepmd/dpmodel/modifier/base_modifier.py (2)
14-22: Check for missing or invalidtypein__new__.While dynamically switching the class is powerful, this snippet assumes
kwargs["type"]is always valid. A fallback or error check for a missing or incorrect"type"key could prevent runtime errors early.
52-73: Ensure friendly error for missing "type" key inget_modifier.Although
modifier_params.pop("type", None)avoids a KeyError, returningNonemight cause issues if a subclass strictly requires a"type". Consider at least validating that a type was provided.source/tests/tf/test_data_modifier.py (1)
10-10: Import path looks good.
The updated import path matches the new module structure and is consistent with the introducedDipoleChargeModifier.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #4621 +/- ##
==========================================
- Coverage 84.81% 84.80% -0.02%
==========================================
Files 688 692 +4
Lines 66294 66343 +49
Branches 3539 3538 -1
==========================================
+ Hits 56230 56263 +33
- Misses 8923 8938 +15
- Partials 1141 1142 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
Could you please tell if the modifier.DipoleChargeModifier is a copy from infer.data_modifier.DipoleChargeModifier ?
If so it seems that you may safely remove infer/data_modifier.py
Yes. It is basically the copy of |
ChiahsinChu
force-pushed
the
devel-modifier-plugin
branch
from
8c86290 to
f9c5d53
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🧹 Nitpick comments (24)
deepmd/pt/entrypoints/main.py (1)
335-362: Consider adding error handling for DDP operations.While the DDP initialization and cleanup are correctly implemented, consider wrapping the training code in a try-finally block to ensure the process group is always destroyed, even if training fails.
# Initialize DDP if os.environ.get("LOCAL_RANK") is not None: dist.init_process_group(backend="cuda:nccl,cpu:gloo") - trainer = get_trainer( - config, - init_model, - restart, - finetune, - force_load, - init_frz_model, - shared_links=shared_links, - finetune_links=finetune_links, - ) - # save min_nbor_dist - if min_nbor_dist is not None: - if not multi_task: - trainer.model.min_nbor_dist = torch.tensor( - min_nbor_dist, dtype=torch.float64, device=DEVICE - ) - else: - for model_item in min_nbor_dist: - trainer.model[model_item].min_nbor_dist = torch.tensor( - min_nbor_dist[model_item], dtype=torch.float64, device=DEVICE - ) - trainer.run() - if dist.is_available() and dist.is_initialized(): - dist.destroy_process_group() + try: + trainer = get_trainer( + config, + init_model, + restart, + finetune, + force_load, + init_frz_model, + shared_links=shared_links, + finetune_links=finetune_links, + ) + # save min_nbor_dist + if min_nbor_dist is not None: + if not multi_task: + trainer.model.min_nbor_dist = torch.tensor( + min_nbor_dist, dtype=torch.float64, device=DEVICE + ) + else: + for model_item in min_nbor_dist: + trainer.model[model_item].min_nbor_dist = torch.tensor( + min_nbor_dist[model_item], dtype=torch.float64, device=DEVICE + ) + trainer.run() + finally: + if dist.is_available() and dist.is_initialized(): + dist.destroy_process_group()deepmd/jax/descriptor/dpa3.py (1)
26-37: Consider adding docstring for clarityWhile the implementation is correct, adding a docstring to the
__setattr__method would improve code maintainability by explaining the purpose of the special handling for different attribute types.def __setattr__(self, name: str, value: Any) -> None: + """Custom attribute setter for JAX compatibility. + + Handles special attributes: + - mean, stddev: Convert to JAX arrays and wrap in ArrayAPIVariable + - repflows: Deserialize and reconstruct DescrptBlockRepflows + - type_embedding: Deserialize and reconstruct TypeEmbedNet + + Parameters + ---------- + name : str + Attribute name + value : Any + Attribute value + """ if name in {"mean", "stddev"}: value = to_jax_array(value) if value is not None: value = ArrayAPIVariable(value) elif name in {"repflows"}: value = DescrptBlockRepflows.deserialize(value.serialize()) elif name in {"type_embedding"}: value = TypeEmbedNet.deserialize(value.serialize()) else: pass return super().__setattr__(name, value)source/tests/pt/model/test_dpa3.py (1)
209-209: Remove or utilize the assigned variablemodel.It appears
modelis assigned but never used. Consider removing the assignment or verifying the compiled model if it's meant for validation or further operations.- model = torch.jit.script(dd0) + torch.jit.script(dd0)🧰 Tools
🪛 Ruff (0.8.2)
209-209: Local variable
modelis assigned to but never usedRemove assignment to unused variable
model(F841)
source/tests/consistent/descriptor/test_dpa3.py (1)
39-43: Simplify the redundant if-else block.Both branches assign
DescrptDPA3PD = None. Consider consolidating it into a single statement or removing the condition altogether.-if INSTALLED_PD: - # not supported yet - DescrptDPA3PD = None -else: - DescrptDPA3PD = None +DescrptDPA3PD = None🧰 Tools
🪛 Ruff (0.8.2)
39-43: Use ternary operator
DescrptDPA3PD = None if INSTALLED_PD else Noneinstead ofif-else-block(SIM108)
deepmd/dpmodel/descriptor/dpa3.py (2)
253-253: Avoid using mutable default argument.Using a mutable list (
[]) as a default can lead to unexpected behavior if it's modified in-place. It's best practice to default toNoneand then assign a list within the function body.- exclude_types: list[tuple[int, int]] = [], + exclude_types: Optional[list[tuple[int, int]]] = None, ... + if exclude_types is None: + exclude_types = []🧰 Tools
🪛 Ruff (0.8.2)
253-253: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
574-574: Remove unused variable or integrate it into the logic.
env_matis never used after being assigned. Consider removing it to maintain cleaner code unless it’s needed for future expansions.-env_mat = repflow_variable.pop("env_mat") +repflow_variable.pop("env_mat")🧰 Tools
🪛 Ruff (0.8.2)
574-574: Local variable
env_matis assigned to but never usedRemove assignment to unused variable
env_mat(F841)
deepmd/dpmodel/descriptor/repflows.py (7)
156-156: Avoid using mutable default arguments.Currently,
exclude_types: list[tuple[int, int]] = []can introduce unexpected behavior if the default list is modified. Consider switching to a non-mutable default, such asNone, and initialize it in the body of the function or constructor.- exclude_types: list[tuple[int, int]] = [] + exclude_types: Optional[list[tuple[int, int]]] = None🧰 Tools
🪛 Ruff (0.8.2)
156-156: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
353-353: Avoid using mutable default arguments.Here,
exclude_types: list[tuple[int, int]] = []should also be replaced with a non-mutable default to prevent potential side effects.- exclude_types: list[tuple[int, int]] = [] + exclude_types: Optional[list[tuple[int, int]]] = None🧰 Tools
🪛 Ruff (0.8.2)
353-353: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
358-469: Consider refactoring thecallmethod into smaller functions.The
callmethod is sizable, handling neighbor-related logic, environment matrix computations, array expansions, and more. Breaking it up into focused sub-methods (e.g.,_prepare_edge_data(),_compute_angle_data()) would enhance clarity and reduce complexity.🧰 Tools
🪛 Ruff (0.8.2)
440-440: Loop control variable
idxnot used within loop bodyRename unused
idxto_idx(B007)
440-440: Unused loop variable.The variable
idxis not used within the loop body. For clarity and to avoid lint warnings, rename it to_idxor remove it if not needed.-for idx, ll in enumerate(self.layers): +for _idx, ll in enumerate(self.layers):🧰 Tools
🪛 Ruff (0.8.2)
440-440: Loop control variable
idxnot used within loop bodyRename unused
idxto_idx(B007)
543-772: Perform deeper testing and potential validation of parameters.The
RepFlowLayerconstructor also has numerous parameters and optional features (e.g., angle compression, multi-edge message). Having more targeted unit tests or parameter validations can help detect misconfigurations early. Consider adding validation checks to confirm consistent parameter usage.
937-937: Unused variable assignment.
nall = node_ebd_ext.shape[1]is never used below. Removing or using the variable meaningfully helps keep the code clear of dead assignments.- nall = node_ebd_ext.shape[1]🧰 Tools
🪛 Ruff (0.8.2)
937-937: Local variable
nallis assigned to but never usedRemove assignment to unused variable
nall(F841)
1199-1199: Remove unused local variable.
nitemis assigned but never used inlist_update_res_residual. Clean up to avoid confusion.- nitem = len(update_list)🧰 Tools
🪛 Ruff (0.8.2)
1199-1199: Local variable
nitemis assigned to but never usedRemove assignment to unused variable
nitem(F841)
source/tests/tf/test_deepmd_data.py (1)
385-445: Provide additional assertions for new multi-type scenarios.The new
TestDataMixTypeclass covers mixed-type initialization and validations. You might consider adding checks for specific behaviors (e.g., loading data with mismatched types) to broaden your test coverage.source/tests/universal/dpmodel/descriptor/test_descriptor.py (2)
21-23: Newly importedRepFlowArgs.This import for
RepFlowArgsis consistent with the newly introduced repflow logic. Ensure that additional arguments inRepFlowArgsare tested thoroughly to maintain coverage.
467-549: Avoid using mutable data structures for default parameters.Line 474 uses
exclude_types=[], which is mutable. Replace this withNoneand initialize a new list inside the function to prevent accidental mutation of the default.- exclude_types=[], + exclude_types=None,🧰 Tools
🪛 Ruff (0.8.2)
474-474: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
deepmd/pt/model/descriptor/repflows.py (2)
178-178: Avoid using mutable default arguments.
exclude_types: list[tuple[int, int]] = []can create shared references if mutated.- exclude_types: list[tuple[int, int]] = [], + exclude_types: Optional[list[tuple[int, int]]] = None, # Then inside __init__: if exclude_types is None: exclude_types = []🧰 Tools
🪛 Ruff (0.8.2)
178-178: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
360-360: Second mutable default argument.
exclude_types: list[tuple[int, int]] = []inreinit_excludemay lead to accidental shared modifications.- def reinit_exclude(self, exclude_types: list[tuple[int, int]] = []): + def reinit_exclude(self, exclude_types: Optional[list[tuple[int, int]]] = None):🧰 Tools
🪛 Ruff (0.8.2)
360-360: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
deepmd/pt/model/descriptor/repflow_layer.py (2)
534-534: Remove unused variable.
nall = node_ebd_ext.shape[1]is declared but never used. Suggest removing it to keep the code clean.- nall = node_ebd_ext.shape[1]🧰 Tools
🪛 Ruff (0.8.2)
534-534: Local variable
nallis assigned to but never usedRemove assignment to unused variable
nall(F841)
788-788: Remove the unused variable.
nitem = len(update_list)is not used afterwards.- nitem = len(update_list)🧰 Tools
🪛 Ruff (0.8.2)
788-788: Local variable
nitemis assigned to but never usedRemove assignment to unused variable
nitem(F841)
deepmd/pt/model/descriptor/dpa3.py (4)
339-339: Rename the unused loop variable to underscore for clarity.Since
iiis not used within the loop body, renaming it to_avoids confusion and signals the variable is intentionally unused.- for ii, descrpt in enumerate(descrpt_list): + for _, descrpt in enumerate(descrpt_list):🧰 Tools
🪛 Ruff (0.8.2)
339-339: Loop control variable
iinot used within loop bodyRename unused
iito_ii(B007)
415-415: Remove the unused variableenv_mat.The variable
env_matis assigned but never used. Removing the assignment prevents clutter in the code.- env_mat = repflow_variable.pop("env_mat")🧰 Tools
🪛 Ruff (0.8.2)
415-415: Local variable
env_matis assigned to but never usedRemove assignment to unused variable
env_mat(F841)
443-443: Correct the spelling in the docstring.Minor spelling fix from "aotm" to "atom."
- The extended aotm types. shape: nf x nall + The extended atom types. shape: nf x nall
471-471: Remove the unused variablenall.Because
nallis never used after its assignment, it's safe to remove the line to reduce extraneous code.- nall = extended_coord.view(nframes, -1).shape[1] // 3🧰 Tools
🪛 Ruff (0.8.2)
471-471: Local variable
nallis assigned to but never usedRemove assignment to unused variable
nall(F841)
🛑 Comments failed to post (2)
deepmd/dpmodel/descriptor/repflows.py (1)
132-210: 🛠️ Refactor suggestion
Consider splitting the constructor for better maintainability.
The
__init__method forDescrptBlockRepflowsis very extensive, featuring numerous parameters and initialization steps. Splitting some setup logic into helper methods (e.g.,_init_embeddings(),_init_layers()) could improve readability, maintainability, and facilitate easier unit testing.🧰 Tools
🪛 Ruff (0.8.2)
156-156: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
deepmd/pt/model/descriptor/dpa3.py (1)
105-105: 🛠️ Refactor suggestion
Avoid mutable default arguments in function signature.
Using a mutable list for the parameter
exclude_typescan lead to unexpected behavior due to aliasing. Instead, consider defaulting toNoneand initializing an empty list inside the function body.- exclude_types: list[tuple[int, int]] = [], + exclude_types: Optional[list[tuple[int, int]]] = None, ... if exclude_types is None: exclude_types = []📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.exclude_types: Optional[list[tuple[int, int]]] = None, ... if exclude_types is None: exclude_types = []🧰 Tools
🪛 Ruff (0.8.2)
105-105: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
…-kit into devel-modifier-plugin
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (26)
deepmd/jax/descriptor/dpa3.py (1)
1-37: Consider adding null checks for serialized attributes.While re-serializing
repflowsandtype_embeddingis likely valid, callingserialize()on aNonevalue or an unexpected type would cause runtime errors. You could handle potential edge cases to make this more robust:elif name in {"repflows"}: - value = DescrptBlockRepflows.deserialize(value.serialize()) + if value is not None: + value = DescrptBlockRepflows.deserialize(value.serialize()) elif name in {"type_embedding"}: - value = TypeEmbedNet.deserialize(value.serialize()) + if value is not None: + value = TypeEmbedNet.deserialize(value.serialize())source/tests/pt/model/test_dpa3.py (1)
141-210: Unused local variablemodelintest_jit.On line 209,
model = torch.jit.script(dd0)is assigned but never used, which the static analysis hints also flagged. If you intend to use JIT-compiled results, consider verifying it within the test or remove the assignment if it's unneeded.Proposed fix (remove unused variable):
- model = torch.jit.script(dd0)🧰 Tools
🪛 Ruff (0.8.2)
209-209: Local variable
modelis assigned to but never usedRemove assignment to unused variable
model(F841)
source/tests/consistent/descriptor/test_dpa3.py (1)
39-43: Remove redundant if-else block.Both branches assign
DescrptDPA3PD = None, making this block a no-op. Consider removing it to simplify the code.-if INSTALLED_PD: - # not supported yet - DescrptDPA3PD = None -else: - DescrptDPA3PD = None +DescrptDPA3PD = None🧰 Tools
🪛 Ruff (0.8.2)
39-43: Use ternary operator
DescrptDPA3PD = None if INSTALLED_PD else Noneinstead ofif-else-block(SIM108)
deepmd/dpmodel/descriptor/dpa3.py (2)
253-253: Use a non-mutable default for theexclude_typesparameter.Defaulting to an empty list can potentially cause unexpected behavior. Instead, default to
Noneand initialize inside the method.-def __init__( - self, - ntypes: int, - # args for repflow - repflow: Union[RepFlowArgs, dict], - # kwargs for descriptor - concat_output_tebd: bool = False, - activation_function: str = "silu", - precision: str = "float64", - exclude_types: list[tuple[int, int]] = [], - env_protection: float = 0.0, - ... -) -> None: +def __init__( + self, + ntypes: int, + repflow: Union[RepFlowArgs, dict], + concat_output_tebd: bool = False, + activation_function: str = "silu", + precision: str = "float64", + exclude_types: Optional[list[tuple[int, int]]] = None, + env_protection: float = 0.0, + ... +) -> None: + if exclude_types is None: + exclude_types = [] self.exclude_types = exclude_types🧰 Tools
🪛 Ruff (0.8.2)
253-253: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
574-574: Remove unused variableenv_mat.The variable
env_matis never used after it is popped fromrepflow_variable, making this assignment redundant.- env_mat = repflow_variable.pop("env_mat")🧰 Tools
🪛 Ruff (0.8.2)
574-574: Local variable
env_matis assigned to but never usedRemove assignment to unused variable
env_mat(F841)
deepmd/dpmodel/descriptor/repflows.py (5)
156-156: Use a non-mutable default forexclude_types.Defaulting to an empty list can potentially cause unexpected behavior. Instead, set it to
Noneand initialize within the constructor.-def __init__( - ..., - exclude_types: list[tuple[int, int]] = [], - env_protection: float = 0.0, - ... -) -> None: +def __init__( + ..., + exclude_types: Optional[list[tuple[int, int]]] = None, + env_protection: float = 0.0, + ... +) -> None: + if exclude_types is None: + exclude_types = [] self.exclude_types = exclude_types🧰 Tools
🪛 Ruff (0.8.2)
156-156: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
353-353: Use a non-mutable default forexclude_types.Similar to the constructor, defaulting to an empty list can lead to unintended side effects. Default to
Noneand handle initialization inside the method.-def reinit_exclude( - self, - exclude_types: list[tuple[int, int]] = [], -) -> None: +def reinit_exclude( + self, + exclude_types: Optional[list[tuple[int, int]]] = None, +) -> None: + if exclude_types is None: + exclude_types = [] self.exclude_types = exclude_types🧰 Tools
🪛 Ruff (0.8.2)
353-353: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
440-440: Rename unused loop variableidx.The loop control variable is not used inside the loop body, so renaming it to
_or_idxclarifies that it is unused.-for idx, ll in enumerate(self.layers): +for _idx, ll in enumerate(self.layers):🧰 Tools
🪛 Ruff (0.8.2)
440-440: Loop control variable
idxnot used within loop bodyRename unused
idxto_idx(B007)
937-937: Remove the unused local variablenall.This variable is assigned but never referenced, so removing it will improve clarity.
-nall = node_ebd_ext.shape[1]🧰 Tools
🪛 Ruff (0.8.2)
937-937: Local variable
nallis assigned to but never usedRemove assignment to unused variable
nall(F841)
1199-1199: Remove unused local variablenitem.This variable is never referenced after being assigned. Removing it clarifies the code.
-nitem = len(update_list)🧰 Tools
🪛 Ruff (0.8.2)
1199-1199: Local variable
nitemis assigned to but never usedRemove assignment to unused variable
nitem(F841)
source/tests/universal/pt/model/test_model.py (1)
99-99: Typo in 'defalut_des_param'.
Consider renaming the variable todefault_des_paramfor improved clarity.- defalut_des_param = [ + default_des_param = [deepmd/tf/modifier/dipole_charge.py (4)
133-148: Variables assigned but never used.
t_mdl_name,t_modi_type,t_mdl_charge_map,t_sys_charge_map,t_ewald_h, andt_ewald_bare all assigned but not utilized. Consider removing these lines to reduce clutter.🧰 Tools
🪛 Ruff (0.8.2)
133-133: Local variable
t_mdl_nameis assigned to but never usedRemove assignment to unused variable
t_mdl_name(F841)
134-134: Local variable
t_modi_typeis assigned to but never usedRemove assignment to unused variable
t_modi_type(F841)
137-137: Local variable
t_mdl_charge_mapis assigned to but never usedRemove assignment to unused variable
t_mdl_charge_map(F841)
142-142: Local variable
t_sys_charge_mapis assigned to but never usedRemove assignment to unused variable
t_sys_charge_map(F841)
147-147: Local variable
t_ewald_his assigned to but never usedRemove assignment to unused variable
t_ewald_h(F841)
148-148: Local variable
t_ewald_bis assigned to but never usedRemove assignment to unused variable
t_ewald_b(F841)
160-160: Unused local variablenfxnas.
This assignment is immediately overwritten. Removing it might keep the code cleaner.🧰 Tools
🪛 Ruff (0.8.2)
160-160: Local variable
nfxnasis assigned to but never usedRemove assignment to unused variable
nfxnas(F841)
390-390: Unused assignment totensor.
This variable is never referenced afterward. Recommend removing to avoid confusion.🧰 Tools
🪛 Ruff (0.8.2)
390-390: Local variable
tensoris assigned to but never usedRemove assignment to unused variable
tensor(F841)
478-478:nframesnot used.
This assignment is never consumed in the code below. You can safely remove it or refactor if needed.🧰 Tools
🪛 Ruff (0.8.2)
478-478: Local variable
nframesis assigned to but never usedRemove assignment to unused variable
nframes(F841)
deepmd/pt/model/descriptor/repflows.py (4)
178-178: Avoid using a mutable default argument for 'exclude_types'.
When using a list as a default parameter, Python reuses the same list object across all calls, which can lead to unexpected behavior.Here is a possible fix:
-def __init__( - self, - ... - exclude_types: list[tuple[int, int]] = [], - ... -): +def __init__( + self, + ... + exclude_types: Optional[list[tuple[int, int]]] = None, + ... +): + if exclude_types is None: + exclude_types = []🧰 Tools
🪛 Ruff (0.8.2)
178-178: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
360-360: Avoid using a mutable default argument for 'exclude_types'.
Same concern arises for thereinit_excludemethod signature.Here is a possible fix:
-def reinit_exclude( - self, - exclude_types: list[tuple[int, int]] = [], -) -> None: +def reinit_exclude( + self, + exclude_types: Optional[list[tuple[int, int]]] = None, +) -> None: + if exclude_types is None: + exclude_types = []🧰 Tools
🪛 Ruff (0.8.2)
360-360: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
461-461: Rename unused loop variable 'idx'.
The variableidxis never used in the loop. For clarity, rename it to_or_idx.-for idx, ll in enumerate(self.layers): +for _, ll in enumerate(self.layers): ...🧰 Tools
🪛 Ruff (0.8.2)
461-461: Loop control variable
idxnot used within loop bodyRename unused
idxto_idx(B007)
571-575: Use a ternary expression for brevity.
Using a ternary expression instead of an if-else statement can make the code more concise.-if callable(merged): - sampled = merged() -else: - sampled = merged +sampled = merged() if callable(merged) else merged🧰 Tools
🪛 Ruff (0.8.2)
571-575: Use ternary operator
sampled = merged() if callable(merged) else mergedinstead ofif-else-block(SIM108)
deepmd/pt/model/descriptor/repflow_layer.py (3)
307-307: Remove or use the unused local variable 'e_dim'.
The variablee_dimis assigned but never used, creating dead code.-nb, nloc, nnei, _ = edge_ebd.shape -e_dim = edge_ebd.shape[-1] +nb, nloc, nnei, e_dim = edge_ebd.shape # If e_dim is truly unneeded, remove its assignment entirely: - e_dim = edge_ebd.shape[-1]🧰 Tools
🪛 Ruff (0.8.2)
307-307: Local variable
e_dimis assigned to but never usedRemove assignment to unused variable
e_dim(F841)
534-534: Remove or use the unused local variable 'nall'.
Currently,nallis assigned but not used, which can add confusion.-nb, nloc, nnei, _ = edge_ebd.shape -nall = node_ebd_ext.shape[1] +nb, nloc, nnei, _ = edge_ebd.shape # Remove nall if it's not needed🧰 Tools
🪛 Ruff (0.8.2)
534-534: Local variable
nallis assigned to but never usedRemove assignment to unused variable
nall(F841)
788-788: Remove or use the unused local variable 'nitem'.
nitemis assigned and not used, leading to unnecessary code.-nitem = len(update_list) uu = update_list[0] for ii in range(1, len(update_list)): ...🧰 Tools
🪛 Ruff (0.8.2)
788-788: Local variable
nitemis assigned to but never usedRemove assignment to unused variable
nitem(F841)
deepmd/pt/model/descriptor/dpa3.py (4)
105-105: Avoid using a mutable default argument for 'exclude_types'.
This repeats the same concern for default-argument lists.-def __init__( - self, - ... - exclude_types: list[tuple[int, int]] = [], - ... -): +def __init__( + self, + ... + exclude_types: Optional[list[tuple[int, int]]] = None, + ... +): + if exclude_types is None: + exclude_types = []🧰 Tools
🪛 Ruff (0.8.2)
105-105: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
339-339: Rename unused loop variable 'ii'.
The loop variableiiis never used in the loop body. For clarity, rename it to_.-for ii, descrpt in enumerate(descrpt_list): +for _, descrpt in enumerate(descrpt_list): descrpt.compute_input_stats(merged, path)🧰 Tools
🪛 Ruff (0.8.2)
339-339: Loop control variable
iinot used within loop bodyRename unused
iito_ii(B007)
415-415: Remove or use the unused local variable 'env_mat'.
This variable is assigned but not used.-env_mat = repflow_variable.pop("env_mat") # Remove the assignment if not needed🧰 Tools
🪛 Ruff (0.8.2)
415-415: Local variable
env_matis assigned to but never usedRemove assignment to unused variable
env_mat(F841)
471-471: Remove or use the unused local variable 'nall'.
The variablenallis assigned but not used, which can clutter the code.-nframes, nloc, nnei = nlist.shape -nall = extended_coord.view(nframes, -1).shape[1] // 3 +... # Remove 'nall' if it's unused in subsequent logic🧰 Tools
🪛 Ruff (0.8.2)
471-471: Local variable
nallis assigned to but never usedRemove assignment to unused variable
nall(F841)
🛑 Comments failed to post (1)
source/tests/universal/dpmodel/descriptor/test_descriptor.py (1)
467-549: 🛠️ Refactor suggestion
Avoid mutable default for
exclude_types.
In Python, using a list directly as a default parameter can lead to unexpected behavior. It is best practice to useNoneand initialize it inside the function body.- def DescriptorParamDPA3( - ntypes, - rcut, - rcut_smth, - sel, - type_map, - env_protection=0.0, - exclude_types=[], - ... - ): + def DescriptorParamDPA3( + ntypes, + rcut, + rcut_smth, + sel, + type_map, + env_protection=0.0, + exclude_types=None, + ... + ): + if exclude_types is None: + exclude_types = [] ...📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.def DescriptorParamDPA3( ntypes, rcut, rcut_smth, sel, type_map, env_protection=0.0, exclude_types=None, update_style="res_residual", update_residual=0.1, update_residual_init="const", update_angle=True, n_multi_edge_message=1, a_compress_rate=0, a_compress_e_rate=1, a_compress_use_split=False, optim_update=True, fix_stat_std=0.3, precision="float64", ): if exclude_types is None: exclude_types = [] input_dict = { # kwargs for repformer "repflow": RepFlowArgs( **{ "n_dim": 20, "e_dim": 10, "a_dim": 8, "nlayers": 2, "e_rcut": rcut, "e_rcut_smth": rcut_smth, "e_sel": sum(sel), "a_rcut": rcut / 2, "a_rcut_smth": rcut_smth / 2, "a_sel": sum(sel) // 4, "a_compress_rate": a_compress_rate, "a_compress_e_rate": a_compress_e_rate, "a_compress_use_split": a_compress_use_split, "optim_update": optim_update, "fix_stat_std": fix_stat_std, "n_multi_edge_message": n_multi_edge_message, "axis_neuron": 2, "update_angle": update_angle, "update_style": update_style, "update_residual": update_residual, "update_residual_init": update_residual_init, } ), "ntypes": ntypes, "concat_output_tebd": False, "precision": precision, "activation_function": "silu", "exclude_types": exclude_types, "env_protection": env_protection, "trainable": True, "use_econf_tebd": False, "use_tebd_bias": False, "type_map": type_map, "seed": GLOBAL_SEED, } return input_dict DescriptorParamDPA3List = parameterize_func( DescriptorParamDPA3, OrderedDict( { "update_residual_init": ("const",), "exclude_types": ([], [[0, 1]]), "update_angle": (True, False), "a_compress_rate": (1,), "a_compress_e_rate": (2,), "a_compress_use_split": (True, False), "optim_update": (True, False), "fix_stat_std": (0.3,), "n_multi_edge_message": (1, 2), "env_protection": (0.0, 1e-8), "precision": ("float64",), } ), ) # to get name for the default function DescriptorParamDPA3 = DescriptorParamDPA3List[0]🧰 Tools
🪛 Ruff (0.8.2)
474-474: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
4 participants
Summary by CodeRabbit
New Features
BaseModifier,DipoleChargeModifier, andDescrptDPA3for better management of modifier functionalities and descriptor computations.model_dpa3for testing purposes.Refactor
Tests
DipoleChargeModifierin test files to reflect new module organization.DescrptDPA3descriptor and added new test classes to validate functionality across different configurations.TestDataMixTypeto validate functionality of theDeepmdDataclass with various type maps.