-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support PIMD for LAMMPS #263
Conversation
Signed-off-by: zjgemi <liuxin_zijian@163.com>
📝 WalkthroughWalkthroughThe changes involve enhancements to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LmpTemplateTaskGroup
participant LmpInput
participant RunLmp
participant MergePIMDFiles
User->>LmpTemplateTaskGroup: set_lmp(numb_models, lmp_template_fname, pimd_bead)
LmpTemplateTaskGroup->>LmpInput: make_lmp_input(pimd_bead)
LmpInput->>RunLmp: execute()
RunLmp->>MergePIMDFiles: merge_pimd_files()
MergePIMDFiles-->>RunLmp: Merged files
RunLmp-->>User: Execution complete
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 (
|
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (12)
dpgen2/constants.py (2)
15-15
: LGTM! Consider adding a comment for clarity.The addition of
lmp_pimd_traj_name
is consistent with the existing naming conventions and supports the PR objective of implementing PIMD for LAMMPS. The use of a placeholder allows for dynamic naming, which is beneficial for PIMD simulations.Consider adding a brief comment to explain the purpose of the placeholder:
# %s placeholder for PIMD bead index or other identifiers lmp_pimd_traj_name = "traj.%s.dump"
18-18
: LGTM! Consider adding a comment for consistency.The addition of
lmp_pimd_model_devi_name
is well-aligned with the existing naming conventions and supports the PR objective of implementing PIMD for LAMMPS. The use of a placeholder is consistent with thelmp_pimd_traj_name
constant.For consistency with the previous suggestion, consider adding a brief comment:
# %s placeholder for PIMD bead index or other identifiers lmp_pimd_model_devi_name = "model_devi.%s.out"dpgen2/exploration/task/npt_task_group.py (1)
52-52
: LGTM! Consider adding documentation for the new parameter.The addition of the
pimd_bead
parameter is well-implemented and consistent with the PR objective of supporting PIMD for LAMMPS. The optional nature and default value maintain backward compatibility.Consider adding a brief description of the
pimd_bead
parameter in the method's docstring to improve code documentation.Also applies to: 76-76
dpgen2/op/run_lmp.py (2)
Line range hint
198-208
: LGTM! Consider adding a comment for clarity.The changes look good and align with the PR objectives. The addition of
merge_pimd_files()
and the handling of electronic temperature data enhance the functionality of theexecute
method.Consider adding a brief comment explaining the purpose of
merge_pimd_files()
for better code readability. For example:# Merge PIMD trajectory and model deviation files merge_pimd_files()
363-375
: Good implementation. Consider adding error handling and cleanup.The
merge_pimd_files
function effectively merges trajectory and model deviation files. The use ofglob
and sorting ensures all relevant files are processed in a consistent order.Consider the following improvements:
- Add error handling for file operations:
def merge_pimd_files(): try: # ... existing code ... except IOError as e: logging.error(f"Error merging PIMD files: {e}") raise
- Add an option to clean up original files after successful merging:
def merge_pimd_files(cleanup=False): # ... existing code ... if cleanup: for file in traj_files + model_devi_files: os.remove(file)
- Consider using
with
statements for file operations to ensure proper closure:with open(lmp_traj_name, "w") as f: for traj_file in sorted(traj_files): with open(traj_file, "r") as f2: f.write(f2.read())dpgen2/exploration/task/make_task_group_from_config.py (2)
50-50
: LGTM! Consider clarifying the documentation.The addition of the
pimd_bead
argument is well-implemented and aligns with the PR objective of supporting PIMD for LAMMPS. The argument is correctly defined as optional with a default value of None.Consider expanding the documentation string to provide more context about PIMD and when this argument should be used. For example:
doc_pimd_bead = "Bead index for Path Integral Molecular Dynamics (PIMD) simulations. Set to None for non-PIMD simulations."Also applies to: 112-118
128-128
: LGTM! Ensure consistency in documentation improvement.The addition of the
pimd_bead
argument to thelmp_template_task_group_args
function is consistent with the changes made innpt_task_group_args
. The implementation is correct and maintains backward compatibility.If you decide to expand the documentation for
pimd_bead
in thenpt_task_group_args
function as suggested earlier, make sure to apply the same improvement here for consistency.Also applies to: 170-176
dpgen2/exploration/task/lmp_template_task_group.py (3)
50-50
: Add docstring forset_lmp
method to includepimd_bead
parameterThe
set_lmp
method now includes a new optional parameterpimd_bead
. Updating the docstring to reflect this change will enhance code readability and maintainability by clearly documenting the purpose and usage of this parameter.
155-161
: Addpimd_bead
parameter torevise_lmp_input_model
docstringThe function
revise_lmp_input_model
now includes an optionalpimd_bead
parameter. Updating the docstring will help users understand the role of this parameter in modifying the LAMMPS input lines for PIMD simulations.
180-184
: Addpimd_bead
parameter torevise_lmp_input_dump
and update docstringThe function
revise_lmp_input_dump
now accepts an optionalpimd_bead
parameter to adjust the trajectory file naming for PIMD simulations. Updating the function's docstring will clarify this new parameter's purpose and usage.dpgen2/exploration/task/lmp/lmp_input.py (1)
54-54
: Consider updating the function documentation to includepimd_bead
parameterThe new optional parameter
pimd_bead
has been added tomake_lmp_input
. Please update the function's docstring to include this parameter and explain its usage.tests/op/test_run_lmp.py (1)
337-339
: Replace bareassert
statements withunittest
assertionsUsing
self.assertEqual
instead of bareassert
statements provides more informative error messages and better integration with theunittest
framework.Apply this diff to make the changes:
- assert len(s) == 4 + self.assertEqual(len(s), 4) - assert model_devi.shape[0] == 4 + self.assertEqual(model_devi.shape[0], 4)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- dpgen2/constants.py (1 hunks)
- dpgen2/exploration/task/lmp/lmp_input.py (5 hunks)
- dpgen2/exploration/task/lmp_template_task_group.py (3 hunks)
- dpgen2/exploration/task/make_task_group_from_config.py (4 hunks)
- dpgen2/exploration/task/npt_task_group.py (3 hunks)
- dpgen2/op/run_lmp.py (3 hunks)
- tests/op/test_run_lmp.py (3 hunks)
🔇 Additional comments (16)
dpgen2/constants.py (1)
Line range hint
1-39
: Summary: PIMD support added with minimal impactThe changes to this file are focused and minimal, adding two new constants (
lmp_pimd_traj_name
andlmp_pimd_model_devi_name
) to support Path Integral Molecular Dynamics (PIMD) for LAMMPS. These additions are consistent with existing naming conventions and do not modify any existing constants, which reduces the risk of breaking changes.The use of placeholders in the new constants allows for dynamic naming, which is likely necessary for PIMD simulations. This flexibility aligns well with the PR objective of supporting PIMD for LAMMPS.
dpgen2/exploration/task/npt_task_group.py (2)
136-136
: LGTM! Consistent implementation of the PIMD feature.The addition of the
pimd_bead
parameter to themake_lmp_input
function call is consistent with the changes in theset_md
method and correctly implements the PIMD feature.
Line range hint
1-139
: Overall, the changes look good and align with the PR objective.The implementation of PIMD support in the
NPTTaskGroup
class is well-executed. The changes are minimal, focused, and maintain backward compatibility. The newpimd_bead
parameter is consistently used across the class methods.dpgen2/op/run_lmp.py (1)
Line range hint
1-375
: Overall, the changes enhance PIMD support as intended.The modifications to the
RunLmp
class and the addition of themerge_pimd_files
function successfully implement support for Path Integral Molecular Dynamics (PIMD) in LAMMPS simulations. These changes align well with the PR objectives and integrate seamlessly with the existing codebase.Key improvements:
- Dynamic merging of trajectory and model deviation files.
- Enhanced handling of electronic temperature data.
The changes are localized and don't introduce any apparent conflicts with other parts of the file.
To ensure the changes don't introduce any unintended side effects, please run the following verification script:
dpgen2/exploration/task/make_task_group_from_config.py (2)
Line range hint
181-281
: Verify ifpimd_bead
argument is needed for customized LMP template.The
pimd_bead
argument has been added tonpt_task_group_args
andlmp_template_task_group_args
, but not tocustomized_lmp_template_task_group_args
.Please confirm if this is intentional or if the
pimd_bead
argument should be added to this function as well for consistency. If it should be added, consider updating the function similar to the other two:def customized_lmp_template_task_group_args(): # ... existing code ... doc_pimd_bead = "Bead index for PIMD, None for non-PIMD" return [ # ... existing arguments ... Argument( "pimd_bead", str, optional=True, default=None, doc=doc_pimd_bead, ), ]
Line range hint
1-281
: Summary: PIMD support added successfully, minor improvements suggestedThe changes to add support for Path Integral Molecular Dynamics (PIMD) in LAMMPS have been implemented correctly in the
npt_task_group_args
andlmp_template_task_group_args
functions. The newpimd_bead
argument is properly defined and documented, maintaining backward compatibility.Key points:
- The implementation is consistent with the PR objective.
- Minor improvements to the documentation have been suggested.
- Verification is needed for the
customized_lmp_template_task_group_args
function to ensure consistency across all relevant functions.Overall, the changes look good and are ready for merge after addressing the minor suggestions and verifying the consistency across all functions.
dpgen2/exploration/task/lmp_template_task_group.py (5)
14-15
: Approved: Importing new constants for PIMD supportThe addition of
lmp_pimd_model_devi_name
alongsidelmp_model_devi_name
correctly imports the necessary constants to support Path Integral Molecular Dynamics (PIMD).
64-67
: Passpimd_bead
consistently to input revision functionsPassing
self.pimd_bead
to bothrevise_lmp_input_model
andrevise_lmp_input_dump
ensures consistent handling of PIMD-specific file naming. This modification appropriately extends the functionality to support PIMD configurations.
171-175
: Approved: Updatedpair_style
configuration with PIMD supportThe modifications to
lmp_lines[idx]
correctly incorporate thepimd_bead
parameter into the model deviation file naming for thepair_style
command. This ensures accurate output file generation during simulations.
56-56
: Initializeself.pimd_bead
before using itEnsure that
self.pimd_bead
is initialized before it is used in subsequent method calls. Although it is assigned here, an explicit check or documentation can prevent potential issues ifset_lmp
is called withoutpimd_bead
.You can verify the initialization with the following script:
✅ Verification successful
self.pimd_bead
is properly initialized before usageThe verification confirms that
self.pimd_bead
is assigned before it is used in subsequent method calls. No instances of usingself.pimd_bead
before its initialization were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if `self.pimd_bead` is used before assignment in the class. # Test: Search for usages of `self.pimd_bead` in methods other than `set_lmp`. # Expect: No usages before assignment in `set_lmp`. rg --python -A 2 'self\.pimd_bead' dpgen2/exploration/task/lmp_template_task_group.pyLength of output: 478
Script:
#!/bin/bash # Description: Check if `self.pimd_bead` is used before assignment in the class. # Search for the assignment of `self.pimd_bead` rg 'self\.pimd_bead\s*=' dpgen2/exploration/task/lmp_template_task_group.py # Search for usages of `self.pimd_bead` excluding the assignment line rg -C 2 'self\.pimd_bead' dpgen2/exploration/task/lmp_template_task_group.py | grep -v 'self\.pimd_bead\s*='Length of output: 709
166-170
: Ensure correct string formatting withpimd_bead
in file namingWhen constructing
model_devi_file_name
, verify thatlmp_pimd_model_devi_name
expects a string formatter compatible withpimd_bead
. This will prevent potential runtime errors ifpimd_bead
isNone
or not the expected type.Use the following script to check the usage:
✅ Verification successful
String formatting for
lmp_pimd_model_devi_name
is correctThe
lmp_pimd_model_devi_name
includes a%s
format specifier, ensuring compatibility withpimd_bead
.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the definition of `lmp_pimd_model_devi_name` and its formatting. # Test: Confirm that `lmp_pimd_model_devi_name` includes a format specifier. # Expect: The constant includes a `%s` or similar for string formatting. rg --python 'lmp_pimd_model_devi_name\s*=\s*' dpgen2/constants.pyLength of output: 478
Script:
#!/bin/bash # Description: Check the definition of `lmp_pimd_model_devi_name` and its formatting. # Test: Confirm that `lmp_pimd_model_devi_name` includes a format specifier. # Expect: The constant includes a `%s` or similar for string formatting. rg 'lmp_pimd_model_devi_name\s*=\s*["\'].*%s.*["\']' dpgen2/constants.pyLength of output: 93
Script:
#!/bin/bash # Description: Check the definition of `lmp_pimd_model_devi_name` and its formatting. # Test: Confirm that `lmp_pimd_model_devi_name` includes a format specifier. # Expect: The constant includes a `%s` or similar for string formatting. rg 'lmp_pimd_model_devi_name\s*=\s*["'"'"'].*%s.*["'"'"']' dpgen2/constants.pyLength of output: 125
dpgen2/exploration/task/lmp/lmp_input.py (5)
15-17
: Imported new constants for PIMD supportThe constants
lmp_pimd_model_devi_name
andlmp_pimd_traj_name
are correctly imported to support Path Integral Molecular Dynamics (PIMD) features.
104-108
: Conditional assignment ofmodel_devi_file_name
based onpimd_bead
The logic for assigning
model_devi_file_name
correctly handles the presence ofpimd_bead
.
111-114
: Ensure correctpair_style
syntax for DeepMD version < 1The
pair_style
command is updated to includemodel_devi_file_name
, which is appropriate for DeepMD versions earlier than 1.0.
128-131
: Updatedpair_style
command for DeepMD version ≥ 1The
pair_style
command correctly incorporatesmodel_devi_file_name
andkeywords
for DeepMD versions 1.0 and above.
140-142
: Conditional assignment oflmp_traj_file_name
based onpimd_bead
The logic for assigning
lmp_traj_file_name
handlespimd_bead
correctly, ensuring the appropriate trajectory file name is used.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #263 +/- ##
==========================================
+ Coverage 83.65% 83.72% +0.06%
==========================================
Files 104 104
Lines 5990 6015 +25
==========================================
+ Hits 5011 5036 +25
Misses 979 979 ☔ View full report in Codecov by Sentry. |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please provide UT for pimd_bead
in exploration/test_lmp_templ_task_group.py
Signed-off-by: zjgemi <liuxin_zijian@163.com>
Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- tests/exploration/test_lmp_templ_task_group.py (3 hunks)
🔇 Additional comments (7)
tests/exploration/test_lmp_templ_task_group.py (7)
200-236
: Verify the consistency of the timestep valueIn the
in_lmp_pimd_template
, thetimestep
is set to0.001
(line 233), whereas in the other templates it is0.002000
. Please verify if this change is intentional and appropriate for PIMD simulations.
261-261
: Ensure proper file naming withmodel_devi.${ibead}.out
In
expected_lmp_pimd_template
, thepair_style
includesout_file model_devi.${ibead}.out
(line 261). Confirm that${ibead}
will be correctly expanded during runtime to distinguish output files for different beads.
267-267
: Confirm the dump file naming conventionThe
dump
command specifiestraj.${ibead}.dump
(line 267). Ensure that this naming convention correctly stores separate trajectory files for each bead and that${ibead}
is properly resolved.
296-297
: Initialize PIMD template file correctlyThe PIMD template file
lmp.pimd.template
is created and written successfully.
303-303
: Ensure cleanup of PIMD template file intearDown
The
os.remove(self.lmp_pimd_template_fname)
statement properly removes the PIMD template file after tests.
426-426
: Verify the usage ofpimd_bead
parameterThe parameter
pimd_bead="${ibead}"
(line 426) is passed toset_lmp
. Confirm that this parameter is correctly utilized within the task group setup and that${ibead}
is properly expanded during task execution.
208-208
:⚠️ Potential issueCheck the
variable ibead
definition for correctnessThe definition
variable ibead uloop 4 pad
may need verification. Ensure that the usage ofuloop
andpad
aligns with LAMMPS variable definitions for intended looping over beads in PIMD simulations.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests