test(pt/dp): add universal uts for all models #3873
Conversation
WalkthroughWalkthroughThe recent updates focus on enhancing the testing framework for Changes
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review due to trivial changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #3873 +/- ##
==========================================
+ Coverage 82.72% 82.88% +0.15%
==========================================
Files 519 519
Lines 50539 50666 +127
Branches 3017 3015 -2
==========================================
+ Hits 41810 41993 +183
+ Misses 7793 7739 -54
+ Partials 936 934 -2 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Outside diff range and nitpick comments (26)
source/tests/universal/dpmodel/model/test_model.py (1)
55-56: Consider setting precision levels for other descriptors if needed.This could help maintain consistency in test expectations across different descriptor types.
deepmd/pt/model/task/dipole.py (2)
Line range hint
82-82: Replace mutable default arguments to avoid potential bugs.- neuron: List[int] = [128, 128, 128], + neuron: Optional[List[int]] = None, + if neuron is None: + neuron = [128, 128, 128]This change avoids the common Python pitfall of mutable default arguments, which can lead to unexpected behavior if the method is called multiple times.
Line range hint
91-91: Address mutable default arguments in the method signature.- exclude_types: List[int] = [], + exclude_types: Optional[List[int]] = None, + if exclude_types is None: + exclude_types = []Similar to the previous issue, this change prevents potential bugs related to mutable default arguments.
deepmd/dpmodel/fitting/dipole_fitting.py (2)
Line range hint
92-92: Address mutable default arguments in the constructor.- neuron: List[int] = [120, 120, 120], + neuron: Optional[List[int]] = None, + if neuron is None: + neuron = [120, 120, 120]This change corrects the mutable default argument issue, ensuring that the default list is not shared across different instances of the class.
Line range hint
105-105: Correct mutable default arguments to ensure safe usage of the method.- trainable: Optional[List[bool]] = None, + trainable: Optional[List[bool]] = None, + if trainable is None: + trainable = [True] * len(neuron) # Assuming the length of neuron is used to determine the number of layersThis modification ensures that the
trainablelist is safely initialized within the method, avoiding shared state issues.deepmd/pt/model/task/polarizability.py (2)
Line range hint
86-86: Avoid using mutable default arguments in function definitions.- neuron: List[int] = [128, 128, 128], + neuron: Optional[List[int]] = None,In the constructor, add:
if neuron is None: neuron = [128, 128, 128]
Line range hint
95-95: Avoid using mutable default arguments in function definitions.- exclude_types: List[int] = [], + exclude_types: Optional[List[int]] = None,In the constructor, add:
if exclude_types is None: exclude_types = []source/tests/universal/pt/model/test_model.py (1)
67-71: Consider removing or revising commented-out code.It's generally a good practice to remove commented-out code to keep the codebase clean and maintainable unless it is intended to be used in the near future.
Also applies to: 116-120, 164-168, 212-216
Tools
GitHub Check: CodeQL
[notice] 67-71: Commented-out code
This comment appears to contain commented-out code.deepmd/dpmodel/fitting/polarizability_fitting.py (2)
Line range hint
97-97: Replace mutable default argument with an immutable one.- neuron: List[int] = [120, 120, 120], + neuron: Optional[List[int]] = None, + ... + if neuron is None: + neuron = [120, 120, 120]
Line range hint
110-110: Replace mutable default argument with an immutable one.- trainable: Optional[List[bool]] = None, + trainable: Optional[List[bool]] = None, + ... + if trainable is None: + trainable = [True] * len(neuron) # Assuming 'neuron' has been defined earliersource/tests/universal/common/cases/descriptor/utils.py (1)
Line range hint
42-42: Rename unused loop variable to indicate it is intentionally unused.- for kk, vv in enumerate(ret[0]): + for kk, _ in enumerate(ret[0]):deepmd/dpmodel/descriptor/se_r.py (3)
Line range hint
105-105: Replace mutable default argument with an immutable one.- neuron: List[int] = [24, 48, 96], + neuron: Optional[List[int]] = None, + ... + if neuron is None: + neuron = [24, 48, 96]
Line range hint
109-109: Replace mutable default argument with an immutable one.- exclude_types: List[List[int]] = [], + exclude_types: Optional[List[List[int]]] = None, + ... + if exclude_types is None: + exclude_types = []
Line range hint
380-380: Remove assignment to unused variable.- env_mat = data.pop("env_mat")deepmd/pt/model/descriptor/se_r.py (6)
Line range hint
64-64: Avoid using mutable default arguments for function parameters.- neuron=[25, 50, 100], + neuron=None, + ... + if neuron is None: + neuron = [25, 50, 100]This change prevents unintended side effects by ensuring that each function call operates with its own default list, rather than sharing it across calls.
Line range hint
69-69: Replace mutable default argument with a safer alternative.- exclude_types: List[Tuple[int, int]] = [], + exclude_types: Optional[List[Tuple[int, int]]] = None, + ... + if exclude_types is None: + exclude_types = []This modification ensures that the default list is not shared between different function calls, which can lead to hard-to-find bugs.
Line range hint
257-261: Utilize a ternary operator for cleaner code.- if callable(merged): - # only get data for once - sampled = merged() - else: - sampled = merged + sampled = merged() if callable(merged) else mergedThis change simplifies the conditional assignment into a single line, improving readability.
Line range hint
297-297: Avoid using mutable default arguments in function parameters.- exclude_types: List[Tuple[int, int]] = [], + exclude_types: Optional[List[Tuple[int, int]]] = None, + ... + if exclude_types is None: + exclude_types = []This change prevents potential bugs caused by the shared mutable default argument across function invocations.
Line range hint
438-438: Remove the unused variableenv_mat.- env_mat = EnvMatStatSe(self)Since
env_matis not used in the subsequent code, removing it avoids unnecessary memory allocation and keeps the code clean.
[!TIP]
Codebase VerificationThe search results show that the
forwardmethod indeepmd/pt/model/descriptor/se_r.pyis called with the parameterss, but there is no mention ofcomm_dict. This indicates that thecomm_dictparameter may not have been integrated properly.
deepmd/pt/model/descriptor/se_r.py: gg = ll.forward(ss)This confirms that the
comm_dictparameter is not being used in theforwardmethod call within this file.Analysis chain
Line range hint
308-323: Verify the integration ofcomm_dictin theforwardmethod.Please ensure that the newly introduced
comm_dictparameter is being appropriately utilized wherever theforwardmethod is called.
The search results show multiple instances of the
forwardmethod across various files, but it does not provide sufficient information to confirm whether thecomm_dictparameter has been integrated properly in all relevant calls.To ensure thorough verification, we need to specifically check the
forwardmethod calls in thedeepmd/pt/model/descriptor/se_r.pyfile and verify if thecomm_dictparameter is being used correctly.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the addition of `comm_dict` is properly handled in all calls to the `forward` method. # Test: Search for the method usage. Expect: All occurrences should now include `comm_dict`. rg --type python $'forward\([^)]*\)'Length of output: 68
Script:
#!/bin/bash # Description: Verify that the addition of `comm_dict` is properly handled in all calls to the `forward` method. # Test: Search for the method usage. Expect: All occurrences should now include `comm_dict`. rg --type py $'forward\([^)]*\)'Length of output: 4652
Script:
#!/bin/bash # Description: Verify the usage of the `comm_dict` parameter in the `forward` method calls within the `deepmd/pt/model/descriptor/se_r.py` file. # Test: Search for the `forward` method calls in the specific file and check if `comm_dict` is included. rg --type py 'forward\([^)]*\)' deepmd/pt/model/descriptor/se_r.pyLength of output: 100
deepmd/dpmodel/output_def.py (2)
Line range hint
107-107: Simplify checking keys in a dictionary.- for kk in self.md.keys_outp(): + for kk in self.md:
Line range hint
445-445: Address unused loop variable.The loop variable
kkis not used within the loop body, which could be a sign of an incomplete implementation or an error.- for kk, vv in def_outp_data.items(): + for _kk, vv in def_outp_data.items():source/tests/universal/common/cases/model/utils.py (1)
22-22: Consider using a relative import for better maintainability.Using relative imports (e.g.,
from ...seed import GLOBAL_SEED) can make the codebase easier to reorganize and maintain. This change is minor but can lead to better scalability and readability in projects with complex directory structures.source/tests/common/dpmodel/test_output_def.py (3)
Line range hint
640-640: Avoid mutable default arguments in function definitions.- def __init__(self, shape=[nf, nloc, 1]): + def __init__(self, shape=None): + self.shape = shape if shape is not None else [nf, nloc, 1] - def __init__(self, shape_rd=[nf, 1], shape_dr=[nf, nall, 1, 3]): + def __init__(self, shape_rd=None, shape_dr=None): + self.shape_rd = shape_rd if shape_rd is not None else [nf, 1] + self.shape_dr = shape_dr if shape_dr is not None else [nf, nall, 1, 3] - def __init__(self, shape=[nf, nloc, 1]): + def __init__(self, shape=None): + self.shape = shape if shape is not None else [nf, nloc, 1]Using mutable default arguments can lead to unexpected behaviors if the function is called multiple times. It's safer to use
Noneas the default value and set the default inside the function.Also applies to: 641-641, 725-725
Line range hint
514-514: Remove unused variable assignment.- with self.assertRaises(ValueError) as context: + with self.assertRaises(ValueError):The variable
contextis assigned but never used within thewithblock. It's better to remove the unused variable to clean up the code.Also applies to: 524-524, 528-528, 541-541, 554-554
Line range hint
693-693: Remove unused variable assignment.- nall = 4The variable
nallis declared but not used in the function. Removing it will clean up the code and avoid confusion.
source/tests/universal/common/cases/data/zbl_tab_potential/H2O_tab_potential.txt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (5)
deepmd/dpmodel/descriptor/se_r.py (2)
Line range hint
105-105: Avoid using mutable default arguments to prevent unintended side effects.- neuron: List[int] = [24, 48, 96], - exclude_types: List[List[int]] = [], + neuron: Optional[List[int]] = None, + exclude_types: Optional[List[List[int]]] = None, ... + self.neuron = neuron if neuron is not None else [24, 48, 96] + self.exclude_types = exclude_types if exclude_types is not None else []Also applies to: 109-109
Line range hint
380-380: Remove the unused variableenv_matto clean up the code.- env_mat = data.pop("env_mat")deepmd/pt/model/descriptor/se_r.py (3)
Line range hint
64-64: Avoid using mutable default arguments to prevent unintended side effects.- neuron=[25, 50, 100], - exclude_types: List[Tuple[int, int]] = [], - exclude_types: List[Tuple[int, int]] = [], + neuron: Optional[List[int]] = None, + exclude_types: Optional[List[Tuple[int, int]]] = None, ... + self.neuron = neuron if neuron is not None else [25, 50, 100] + self.exclude_types = exclude_types if exclude_types is not None else []Also applies to: 69-69, 297-297
Line range hint
257-261: Simplify the conditional assignment using a ternary operator.- if callable(merged): - sampled = merged() - else: - sampled = merged + sampled = merged() if callable(merged) else merged
Line range hint
438-438: Remove the unused variableenv_matto clean up the code.- env_mat = data.pop("env_mat")
source/tests/universal/common/cases/data/zbl_tab_potential/H2O_tab_potential.txt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 25
Outside diff range and nitpick comments (18)
deepmd/utils/path.py (1)
Line range hint
415-415: Consider using a more descriptive variable name instead ofl.- l = [] + keys_list = []deepmd/dpmodel/descriptor/se_r.py (3)
Line range hint
109-109: Replace mutable default arguments with immutable ones to avoid potential bugs.- neuron: List[int] = [24, 48, 96], - exclude_types: List[List[int]] = [], + neuron: Optional[List[int]] = None, + exclude_types: Optional[List[List[int]]] = None,And then initialize these within the function:
if neuron is None: neuron = [24, 48, 96] if exclude_types is None: exclude_types = []Also applies to: 113-113
Line range hint
385-385: Remove the unused local variableenv_mat.- env_mat = data.pop("env_mat")This variable is declared but not used, which could lead to confusion and clutter in the code.
Line range hint
385-385: Clarify the handling of non-implemented features in the class.The class contains several features marked as not implemented. It would be beneficial to provide more detailed error messages or documentation that explains why these features are not implemented and under what conditions they might be in the future.
deepmd/dpmodel/model/spin_model.py (1)
Line range hint
27-174: Ensure proper documentation for theSpinModelclass methods.The methods in the
SpinModelclass lack detailed docstrings, particularly describing the parameters and the return values. This can make the code harder to understand and maintain. Consider adding comprehensive docstrings to improve code readability and maintainability.deepmd/pt/model/descriptor/se_r.py (3)
Line range hint
67-67: Avoid mutable default arguments in function definitions.Using mutable default arguments such as lists or dictionaries can lead to unexpected behavior if the function modifies these arguments. Default arguments are evaluated only once at function definition time, which means that the default value becomes shared between all calls.
- exclude_types: List[Tuple[int, int]] = [], + exclude_types: Optional[List[Tuple[int, int]]] = None,And then inside the function:
if exclude_types is None: exclude_types = []
Line range hint
260-264: Use a ternary operator for conditional assignments to simplify the code.The conditional assignment of
sampledcan be simplified using a ternary operator, which makes the code more concise and easier to read.- if callable(merged): - sampled = merged() - else: - sampled = merged + sampled = merged() if callable(merged) else merged
Line range hint
311-326: Ensure proper handling of optional parameters in function signatures.The function signature for
forwardincludescomm_dictas an optional parameter but does not handle the case where it might beNone. It's good practice to explicitly check forNonebefore using such parameters.if comm_dict is not None: # Use comm_dictdeepmd/dpmodel/output_def.py (2)
Line range hint
107-107: Optimize thekeys_outpmethod call.You can simplify the
keys_outpmethod call by removing.keys()since it's redundant when checking membership in dictionaries:- if key in self.var_defs.keys(): + if key in self.var_defs:This change makes the code cleaner and slightly more efficient.
Line range hint
450-450: Address unused loop variable.The loop control variable
kkis not used within the loop body, which could be misleading. Consider renaming it to_to indicate its non-use:- for kk, vv in def_outp_data.items(): + for _, vv in def_outp_data.items():This change clarifies that the key is not used within the loop.
deepmd/pt/model/model/spin_model.py (1)
Line range hint
393-393: Usekey in dictinstead ofkey in dict.keys().- if "mask" in model_output_type.keys(): + if "mask" in model_output_type:source/tests/universal/common/cases/atomic_model/utils.py (2)
44-57: Consider adding docstrings for the newly added class attributes to maintain consistency and improve code readability.
100-106: Ensure consistent indentation and spacing within thetest_has_message_passingmethod for improved readability.source/tests/common/dpmodel/test_output_def.py (4)
763-775: The implementation oftest_squeezeappears correct, but consider adding more thorough testing for edge cases.For robustness, you might want to add tests for high-dimensional tensors and tensors where multiple dimensions can be squeezed.
# Example additional test cases: out_var = OutputVariableDef("foo", [1, 1, 1]) out_var.squeeze() # Squeeze all singleton dimensions self.assertEqual(out_var.shape, []) out_var = OutputVariableDef("foo", [2, 1, 2]) out_var.squeeze() # Should only squeeze the middle dimension self.assertEqual(out_var.shape, [2, 2])
Line range hint
482-482: Remove unused variablecontextin exception blocks.- with self.assertRaises(ValueError) as context: + with self.assertRaises(ValueError):Apply this change to all similar instances in the file.
Also applies to: 492-492, 496-496, 509-509, 522-522
Line range hint
608-608: Avoid using mutable default arguments in function definitions.- def __init__(self, shape_rd=[nf, 1], shape_dr=[nf, nall, 1, 3]): + def __init__(self, shape_rd=None, shape_dr=None): + self.shape_rd = shape_rd if shape_rd is not None else [nf, 1] + self.shape_dr = shape_dr if shape_dr is not None else [nf, nall, 1, 3]Apply this change to all similar instances in the file.
Also applies to: 609-609, 693-693
Line range hint
661-661: Remove the unused variablenall.- nall = 4source/tests/universal/common/cases/model/utils.py (1)
Line range hint
183-183: Use direct key check instead of calling.keys()for dictionary membership testing.- if kk in self.output_def.keys(): + if kk in self.output_def:
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (6)
deepmd/dpmodel/output_def.py (2)
Line range hint
107-107: Optimize dictionary key lookup inmodel_check_output.- for kk in self.md.keys_outp(): + for kk in self.md.keys_outp:This change removes the unnecessary call to
.keys()and directly iterates over the dictionary keys, improving performance.
Line range hint
450-450: Rename unused loop control variable indo_mask.- for kk, vv in def_outp_data.items(): + for _kk, vv in def_outp_data.items():Renaming
kkto_kkclarifies that this variable is not used within the loop body, adhering to Python conventions for unused variables.deepmd/pt/model/model/spin_model.py (1)
Line range hint
393-393: Optimize dictionary key lookup inmodel_output_def.- if "mask" in model_output_type: + if "mask" in model_output_type.keys():This change removes the unnecessary call to
.keys()and directly checks for the key in the dictionary, improving performance.source/tests/common/dpmodel/test_output_def.py (3)
Line range hint
482-482: Remove unused local variablecontext.- with self.assertRaises(ValueError) as context: + with self.assertRaises(ValueError):Also applies to: 492-492, 496-496, 509-509, 522-522
Line range hint
608-608: Replace mutable default arguments withNoneto avoid potential bugs.- def __init__(self, shape_rd=[nf, 1], shape_dr=[nf, nall, 1, 3]): + def __init__(self, shape_rd=None, shape_dr=None): + if shape_rd is None: + shape_rd = [nf, 1] + if shape_dr is None: + shape_dr = [nf, nall, 1, 3] - def __init__(self, shape=[nf, nloc, 1]): + def __init__(self, shape=None): + if shape is None: + shape = [nf, nloc, 1]Also applies to: 609-609, 693-693
Line range hint
661-661: Remove unused variablenall.- nall = 4
There was a problem hiding this comment.
Actionable comments posted: 13
Outside diff range and nitpick comments (2)
deepmd/pt/utils/exclude_mask.py (1)
Line range hint
22-22: Replace mutable default arguments with None.Using mutable default arguments can lead to unexpected behaviors if the arguments are modified. Replace them with
Noneand initialize within the function.- def __init__(self, ntypes: int, exclude_types: List[int] = []): + def __init__(self, ntypes: int, exclude_types: List[int] = None): + if exclude_types is None: + exclude_types = [] - def reinit(self, ntypes: int, exclude_types: List[int] = []): + def reinit(self, ntypes: int, exclude_types: List[int] = None): + if exclude_types is None: + exclude_types = []Also applies to: 30-30, 75-75, 83-83
source/tests/universal/common/cases/model/utils.py (1)
Line range hint
194-194: Refactor dictionary key checks for better readability and performanceAs suggested by the static analysis tool, use
key in dictinstead ofkey in dict.keys()for better performance and readability.- if kk in self.output_def.keys(): + if kk in self.output_def:
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
deepmd/pt/model/task/polarizability.py (1)
Line range hint
86-86: Avoid mutable default arguments.Using mutable default values for function arguments can lead to unexpected behavior, as modifications to these arguments will persist across function calls. Consider using
Noneas a default value and initializing within the function if necessary.- neuron: List[int] = [128, 128, 128], - scale: Optional[Union[List[float], float]] = None, + neuron: Optional[List[int]] = None, + scale: Optional[Union[List[float], float]] = None,In the method definitions, add checks to initialize these if they are
None.Also applies to: 95-95
deepmd/pt/model/descriptor/hybrid.py (1)
Line range hint
176-176: Minor refactoring suggested for unused loop variables.The loop variables
desandiiare not used within their respective loops and should be renamed to_to indicate that they are unused.- for ii, des in enumerate(self.descrpt_list): + for _, _ in enumerate(self.descrpt_list):Also applies to: 220-220
deepmd/pt/model/model/spin_model.py (1)
Line range hint
396-396: Use key checks directly on dictionaries.It's more pythonic and efficient to check for key existence directly using
key in dictrather thankey in dict.keys().- if "key" in dict.keys(): + if "key" in dict:
Add universal uts for all models/atomic models: - pt/dp: all atomic models for permutation/translation/rotation/smooth tests - pt: all standard models for permutation/translation/rotation/smooth/autodiff tests - dp: energy model for permutation/translation/rotation/smooth tests Add `rot_invariant` outdef attribute to automately determine if the variable is rotationally invariant. Add `translated_output_def` to pt models to get translated outdef variable. Fix bugs when adding ut: - linear atomic model typos in mapping - mask in dp linear atomic model - comm_dict forward in se_r descriptor In progressing (later merged in this PR): - [x] UT for SpinEnerModel **May need help:** script module API test in model uts are excluded because of long processing time. I've tried to only get torch.jit.script once, but it still doesn't work. **Note** force/virial autodiff/rot test in dipole/polar need further deduction. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Tests** - Enhanced precision and accuracy of various model tests, including permutation, translation, rotation, smoothing, and autodiff. - Introduced comprehensive test cases for atomic models like energy, dipole, DOS, polarizability, and ZBL. - Improved descriptor and fitting parameter tests with new functions and parameterizations. - Added test cases for energy and spin models in a deep learning framework. - Implemented conditional test skips based on the `TEST_DEVICE` value. - **Chores** - Configured `CUDA_VISIBLE_DEVICES` for GitHub Actions to ensure tests run on the correct hardware. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Add universal uts for all models/atomic models:
Add
rot_invariantoutdef attribute to automately determine if the variable is rotationally invariant.Add
translated_output_defto pt models to get translated outdef variable.Fix bugs when adding ut:
In progressing (later merged in this PR):
May need help:
script module API test in model uts are excluded because of long processing time. I've tried to only get torch.jit.script once, but it still doesn't work.
Note
force/virial autodiff/rot test in dipole/polar need further deduction.
Summary by CodeRabbit
Tests
TEST_DEVICEvalue.Chores
CUDA_VISIBLE_DEVICESfor GitHub Actions to ensure tests run on the correct hardware.