feat pt : Support property fitting #3867
Conversation
WalkthroughWalkthroughThe recent updates enhance the DeepMD framework by introducing new classes and methods focused on property fitting and evaluation. Key additions include the Changes
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional comments not posted (2)
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 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: 3
Outside diff range and nitpick comments (8)
deepmd/pt/train/training.py (6)
Line range hint
366-366: Use the simplified method to access dictionary values.- config.get("learning_rate_dict", None) + config.get("learning_rate_dict")Also applies to: 452-452
Line range hint
470-470: Remove assignments to unused variables to clean up the code.- ntest = model_params.get("data_bias_nsample", 1) - old_type_map, new_type_map = ( - _model_params["type_map"], - _model_params["new_type_map"], - )Also applies to: 586-586
Line range hint
688-688: Use context managers for file operations to ensure proper resource management.- fout = open(self.disp_file, mode="w", buffering=1) + with open(self.disp_file, mode="w", buffering=1) as fout: - fout1 = open(record_file, mode="w", buffering=1) + with open(record_file, mode="w", buffering=1) as fout1:Also applies to: 692-692
Line range hint
734-737: Simplify the conditional assignment using a ternary operator.- if _step_id < self.warmup_steps: - pref_lr = _lr.start_lr - else: - pref_lr = cur_lr + pref_lr = _lr.start_lr if _step_id < self.warmup_steps else cur_lr
Line range hint
844-844: Rename the unused loop control variable to_to indicate it is intentionally unused.- for ii in range(valid_numb_batch): + for _ in range(valid_numb_batch):
Line range hint
1115-1115: Use direct key checks in dictionaries instead of checking against the keys list.- if key in dict.keys(): + if key in dict:deepmd/utils/argcheck.py (2)
Line range hint
75-75: Specifystacklevelinwarnings.warnto improve the clarity of the warning's origin.- warnings.warn(f"{key} has been removed and takes no effect.", FutureWarning) + warnings.warn(f"{key} has been removed and takes no effect.", FutureWarning, stacklevel=2)Tools
Ruff
2536-2536: Local variable
baseis assigned to but never used (F841)Remove assignment to unused variable
base
Line range hint
1171-1171: Remove unused variable assignments to clean up the code.- link_lf = make_link("loc_frame", "model/descriptor[loc_frame]") - link_se_e2_a = make_link("se_e2_a", "model/descriptor[se_e2_a]") - link_se_e2_r = make_link("se_e2_r", "model/descriptor[se_e2_r]") - link_se_e3 = make_link("se_e3", "model/descriptor[se_e3]") - link_se_a_tpe = make_link("se_a_tpe", "model/descriptor[se_a_tpe]") - link_hybrid = make_link("hybrid", "model/descriptor[hybrid]") - link_se_atten = make_link("se_atten", "model/descriptor[se_atten]") - link_se_atten_v2 = make_link("se_atten_v2", "model/descriptor[se_atten_v2]")Also applies to: 1172-1172, 1173-1173, 1174-1174, 1175-1175, 1176-1176, 1177-1177, 1178-1178
Tools
Ruff
2536-2536: Local variable
baseis assigned to but never used (F841)Remove assignment to unused variable
base
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #3867 +/- ##
==========================================
+ Coverage 83.01% 83.06% +0.05%
==========================================
Files 524 532 +8
Lines 51642 51971 +329
Branches 3030 3030
==========================================
+ Hits 42871 43171 +300
- Misses 7825 7855 +30
+ Partials 946 945 -1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (10)
deepmd/pt/utils/stat.py (4)
Line range hint
88-88: Avoid using mutable data structures for default arguments.- def compute_output_stats(merged, ntypes, keys=["energy"], stat_file_path=None, rcond=None, preset_bias=None, model_forward=None, intensive=False): + def compute_output_stats(merged, ntypes, keys=None, stat_file_path=None, rcond=None, preset_bias=None, model_forward=None, intensive=False): + if keys is None: + keys = ["energy"]
Line range hint
154-154: Remove the unused variablenatomsto clean up the code.- natoms = system["natoms"],
Line range hint
238-238: Avoid using mutable data structures for default arguments.- def compute_output_stats_global(sampled, ntypes, keys=["energy"], rcond=None, preset_bias=None, model_pred=None, intensive=False): + def compute_output_stats_global(sampled, ntypes, keys=None, rcond=None, preset_bias=None, model_pred=None, intensive=False): + if keys is None: + keys = ["energy"]
Line range hint
292-292: Usekey in dictinstead ofkey in dict.keys()for a more Pythonic and efficient approach.- if kk in preset_bias.keys() + if kk in preset_bias - for kk in keys.keys() + for kk in keys - for kk in bias_atom_e.keys() + for kk in bias_atom_e - for kk in merged_natoms.keys() + for kk in merged_natoms - for kk in model_pred.keys() + for kk in model_predAlso applies to: 335-335, 344-344, 449-449, 491-491, 497-497, 499-499, 504-504
deepmd/pt/train/training.py (6)
Line range hint
367-367: Useconfig.get("learning_rate_dict")instead ofconfig.get("learning_rate_dict", None)as it is cleaner and the default return ofgetisNone.- config.get("learning_rate_dict", None) + config.get("learning_rate_dict")Also applies to: 453-453
Line range hint
471-471: Remove assignment to unused variablesntestandold_type_map, which are declared but never used.- ntest = model_params.get("data_bias_nsample", 1) - old_type_map, new_type_map = ( - _model_params["type_map"], - _model_params["new_type_map"], - )Also applies to: 587-587
Line range hint
526-526: Usekey in dictinstead ofkey in dict.keys()for checking key existence in a dictionary, which is more Pythonic and efficient.- key in dict.keys() + key in dictAlso applies to: 1116-1116
Line range hint
689-689: Use context handlers for file operations to ensure that files are properly closed after their scope ends, which improves code safety and readability.- fout = open(self.disp_file, mode="w", buffering=1) + with open(self.disp_file, mode="w", buffering=1) as fout: - fout1 = open(record_file, mode="w", buffering=1) + with open(record_file, mode="w", buffering=1) as fout1:Also applies to: 693-693
Line range hint
735-738: Simplify the conditional assignment forpref_lrusing a ternary operator, which makes the code more concise.- if _step_id < self.warmup_steps: - pref_lr = _lr.start_lr - else: - pref_lr = cur_lr + pref_lr = _lr.start_lr if _step_id < self.warmup_steps else cur_lr
Line range hint
845-845: Rename the unused loop control variableiito_to indicate that it is intentionally unused.- for ii in range(valid_numb_batch): + for _ in range(valid_numb_batch):
for more information, see https://pre-commit.ci
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range and nitpick comments (16)
deepmd/pt/utils/stat.py (4)
Line range hint
88-88: Avoid using mutable default structures for argument defaults. Replace withNoneand initialize within the function.- preset_bias: Optional[Dict[str, List[Optional[torch.Tensor]]]] = {}, + preset_bias: Optional[Dict[str, List[Optional[torch.Tensor]]]] = None, + if preset_bias is None: + preset_bias = {},Also applies to: 238-238
Line range hint
154-154: Remove the unused variablenatoms.- natoms = {kk: merged_natoms[kk].sum(-1) for kk in bias_atom_e.keys()}
Line range hint
166-166: Ensure loop variablesnframesandsystemare bound in the function definition.- for kk in keys: + for kk, nframes, system in zip(keys, nf, sampled):Also applies to: 167-167
Line range hint
292-292: Replacekey in dict.keys()withkey in dictto simplify the code.- if kk in preset_bias.keys() + if kk in preset_biasAlso applies to: 335-335, 344-344, 449-449, 493-493, 499-499, 501-501, 506-506
deepmd/pt/model/atomic_model/base_atomic_model.py (3)
Line range hint
80-80: Avoid using mutable default arguments.- def __init__(self, type_map: List[str], atom_exclude_types: List[int] = [], pair_exclude_types: List[Tuple[int, int]] = [], rcond: Optional[float] = None, preset_out_bias: Optional[Dict[str, torch.Tensor]] = None): + def __init__(self, type_map: List[str], atom_exclude_types: List[int] = None, pair_exclude_types: List[Tuple[int, int]] = None, rcond: Optional[float] = None, preset_out_bias: Optional[Dict[str, torch.Tensor]] = None): + atom_exclude_types = atom_exclude_types if atom_exclude_types is not None else [] + pair_exclude_types = pair_exclude_types if pair_exclude_types is not None else [] - def reinit_atom_exclude(self, exclude_types: List[int] = []): + def reinit_atom_exclude(self, exclude_types: List[int] = None): + exclude_types = exclude_types if exclude_types is not None else [] - def reinit_pair_exclude(self, exclude_types: List[Tuple[int, int]] = []): + def reinit_pair_exclude(self, exclude_types: List[Tuple[int, int]] = None): + exclude_types = exclude_types if exclude_types is not None else []Also applies to: 81-81, 129-129, 139-139
Line range hint
95-95: Remove unused local variable.- ntypes = self.get_ntypes()
Line range hint
254-254: Optimize dictionary key checks.- for kk in ret_dict.keys(): + for kk in ret_dict: - if kk in out_std.keys() + if kk in out_std - if kk in out_bias.keys() + if kk in out_biasAlso applies to: 543-543, 544-544
deepmd/pt/train/training.py (7)
Line range hint
375-375: Useconfig.get("learning_rate_dict")instead ofconfig.get("learning_rate_dict", None)for simplicity.- if self.multi_task and config.get("learning_rate_dict", None) is not None: + if self.multi_task and config.get("learning_rate_dict") is not None:
Line range hint
469-469: Simplify the check by usingconfig.get("learning_rate_dict").- if self.multi_task and config.get("learning_rate_dict", None) is not None: + if self.multi_task and config.get("learning_rate_dict") is not None:
Line range hint
567-567: Usekey in dictinstead ofkey in dict.keys()to check for key existence in a dictionary.- missing_keys = [key for key in self.model_keys if key not in self.optim_dict.keys()] + missing_keys = [key for key in self.model_keys if key not in self.optim_dict]
Line range hint
714-714: Use a context manager when opening files to ensure that resources are properly managed and the file is closed after its contents are no longer needed.- fout = open(self.disp_file, mode="w", buffering=1) if self.rank == 0 else None + fout = open(self.disp_file, mode="w", buffering=1) if self.rank == 0 else None # Consider using `with open(...) as fout:` to ensure file closure - fout1 = open(record_file, mode="w", buffering=1) + fout1 = open(record_file, mode="w", buffering=1) # Consider using `with open(...) as fout1:` to ensure file closureAlso applies to: 718-718
Line range hint
760-763: Refactor the if-else block to a ternary operator for clarity and brevity.- if _step_id < self.warmup_steps: - pref_lr = _lr.start_lr - else: - pref_lr = cur_lr + pref_lr = _lr.start_lr if _step_id < self.warmup_steps else cur_lr
Line range hint
870-870: Rename the unused loop variableiito_iito indicate it's intentionally unused.- for ii in range(valid_numb_batch): + for _ii in range(valid_numb_batch):
Line range hint
1141-1141: Usekey in dictinstead ofkey in dict.keys()for checking key existence.- missing_keys = [item for item in target_keys if item not in input_keys.keys()] + missing_keys = [item for item in target_keys if item not in input_keys]deepmd/utils/argcheck.py (2)
Line range hint
75-75: Specify astacklevelfor the warning to improve debugging.- warnings.warn(f"{key} has been removed and takes no effect.", FutureWarning) + warnings.warn(f"{key} has been removed and takes no effect.", FutureWarning, stacklevel=2)This change will help trace back the warning to the caller of the function, rather than the utility function itself, which can be very helpful in debugging.
Tools
Ruff
2536-2536: Local variable
baseis assigned to but never used (F841)Remove assignment to unused variable
base
Line range hint
1171-1178: Remove unused local variables that store link references.- link_lf = make_link("loc_frame", "model/descriptor[loc_frame]") - link_se_e2_a = make_link("se_e2_a", "model/descriptor[se_e2_a]") - link_se_e2_r = make_link("se_e2_r", "model/descriptor[se_e2_r]") - link_se_e3 = make_link("se_e3", "model/descriptor[se_e3]") - link_se_a_tpe = make_link("se_a_tpe", "model/descriptor[se_a_tpe]") - link_hybrid = make_link("hybrid", "model/descriptor[hybrid]") - link_se_atten = make_link("se_atten", "model/descriptor[se_atten]") - link_se_atten_v2 = make_link("se_atten_v2", "model/descriptor[se_atten_v2]")These variables are assigned but never used, which can lead to unnecessary memory usage and potential confusion.
Tools
Ruff
2536-2536: Local variable
baseis assigned to but never used (F841)Remove assignment to unused variable
base
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (4)
deepmd/pt/model/atomic_model/base_atomic_model.py (4)
Line range hint
80-80: Replace mutable default argument withNone.- def __init__(self, type_map: List[str], atom_exclude_types: List[int] = [], pair_exclude_types: List[Tuple[int, int]] = [], rcond: Optional[float] = None, preset_out_bias: Optional[Dict[str, torch.Tensor]] = None): + def __init__(self, type_map: List[str], atom_exclude_types: List[int] = None, pair_exclude_types: List[Tuple[int, int]] = None, rcond: Optional[float] = None, preset_out_bias: Optional[Dict[str, torch.Tensor]] = None): + atom_exclude_types = atom_exclude_types if atom_exclude_types is not None else [] + pair_exclude_types = pair_exclude_types if pair_exclude_types is not None else []
Line range hint
129-129: Replace mutable default arguments withNone.- def reinit_atom_exclude(self, exclude_types: List[int] = []): - def reinit_pair_exclude(self, exclude_types: List[Tuple[int, int]] = []): + def reinit_atom_exclude(self, exclude_types: List[int] = None): + exclude_types = exclude_types if exclude_types is not None else [] + def reinit_pair_exclude(self, exclude_types: List[Tuple[int, int]] = None): + exclude_types = exclude_types if exclude_types is not None else []Also applies to: 139-139
Line range hint
254-254: Optimize dictionary key checks.- if key in self.bias_keys.keys(): + if key in self.bias_keys:Also applies to: 548-548, 549-549
Line range hint
95-95: Remove unused local variablentypes.- ntypes = self.get_ntypes()
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (2)
deepmd/utils/argcheck.py (2)
Line range hint
75-75: Please specify astacklevelfor thewarnings.warncall to improve debugging.- warnings.warn(f"{key} has been removed and takes no effect.", FutureWarning) + warnings.warn(f"{key} has been removed and takes no effect.", FutureWarning, stacklevel=2)
Line range hint
1171-1178: Remove unused variable assignments.- link_lf = make_link("loc_frame", "model/descriptor[loc_frame]") - link_se_e2_a = make_link("se_e2_a", "model/descriptor[se_e2_a]") - link_se_e2_r = make_link("se_e2_r", "model/descriptor[se_e2_r]") - link_se_e3 = make_link("se_e3", "model/descriptor[se_e3]") - link_se_a_tpe = make_link("se_a_tpe", "model/descriptor[se_a_tpe]") - link_hybrid = make_link("hybrid", "model/descriptor[hybrid]") - link_se_atten = make_link("se_atten", "model/descriptor[se_atten]") - link_se_atten_v2 = make_link("se_atten_v2", "model/descriptor[se_atten_v2]")
wanghan-iapcm
changed the title
devel branch
I see that 1/3 of the new codes have not been tested. |
Where can I see these datas about missing UT? I will add UT based on these datas. |
for more information, see https://pre-commit.ci
You can click the link sent by @codecov or click the codecov checks. It looks like the loss module is not tested. @iProzd Do we have a universal test fixture for loss functions? |
Not yet, maybe we need a discussion to design a universal test for loss modules. |
Fix the spelling as suggested by deepmodeling#3867 (comment). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Corrected typos in attribute names from `reduciable` to `reducible` across multiple files, enhancing the accuracy of parameter definitions and improving code consistency. - **Tests** - Updated test cases to reflect the corrected attribute names, ensuring that tests accurately validate the new `reducible` parameter. These changes improve the clarity and correctness of the codebase, ensuring that attribute names are consistent and accurately reflect their intended functionality. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Solve issue deepmodeling#3866 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced property fitting neural networks with the new `PropertyFittingNet` class. - Added the `DeepProperty` class for evaluating properties of structures using a deep learning model. - Implemented the `PropertyModel` class to integrate properties specific to atomic models. - **Enhancements** - Added an `intensive` property to several classes to indicate whether a fitting property is intensive or extensive. - Enhanced output property definitions and manipulation methods in various models for improved property evaluation. - Expanded loss function capabilities to handle the "property" loss type. - Improved argument definitions for fitting and loss functionalities, enhancing configurability. - Updated the model selection logic to include the new `PropertyModel`. - Enhanced the `DeepEvalWrapper` class to support additional model evaluation features, including new methods for retrieving model characteristics. - **Documentation** - Updated class docstrings to reflect new attributes and parameters, improving clarity and usability. - **Tests** - Expanded the set of test examples related to the "property" category to improve test coverage. - Introduced new test classes and parameterized tests for improved validation of property-related functionalities. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Chenqqian Zhang <100290172+Chengqian-Zhang@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Solve issue #3866
Summary by CodeRabbit
New Features
PropertyFittingNetclass.DeepPropertyclass for evaluating properties of structures using a deep learning model.PropertyModelclass to integrate properties specific to atomic models.Enhancements
intensiveproperty to several classes to indicate whether a fitting property is intensive or extensive.PropertyModel.DeepEvalWrapperclass to support additional model evaluation features, including new methods for retrieving model characteristics.Documentation
Tests