-
Notifications
You must be signed in to change notification settings - Fork 520
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
fix dplr: support initializing from a restart file #4084
Conversation
📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes involve renaming the Changes
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 (
|
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: 0
Outside diff range, codebase verification and nitpick comments (2)
source/lmp/fix_dplr.h (1)
47-47
: Incomplete Renaming DetectedThe method
post_integrate()
is still being referenced insource/lmp/fix_dplr.cpp
. Please ensure that all occurrences ofpost_integrate()
are updated toend_of_step()
to maintain consistency across the codebase.
- File:
source/lmp/fix_dplr.cpp
- Line: Reference found in
min_pre_exchange()
method.Analysis chain
Method Renaming Approved
The renaming of
post_integrate()
toend_of_step()
is appropriate for the described functionality. Ensure that all references to this method in the codebase are updated accordingly.The code changes are approved.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to `post_integrate` are updated to `end_of_step`. # Test: Search for the old function name. Expect: No occurrences. rg --type cpp -A 5 $'post_integrate'Length of output: 375
source/lmp/fix_dplr.cpp (1)
402-402
: Incomplete Renaming DetectedThe function
post_integrate
is still being referenced in themin_pre_exchange
method withinsource/lmp/fix_dplr.cpp
. Please update all occurrences ofpost_integrate
toend_of_step
to ensure consistency and prevent potential errors.
- File:
source/lmp/fix_dplr.cpp
- Line:
void FixDPLR::min_pre_exchange() { post_integrate(); }
Analysis chain
Method Implementation and Renaming Approved
The implementation of
end_of_step()
correctly updates the WC positions as intended. Ensure that all references to this method in the codebase are updated accordingly.The code changes are approved.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to `post_integrate` are updated to `end_of_step`. # Test: Search for the old function name. Expect: No occurrences. rg --type cpp -A 5 $'post_integrate'Length of output: 375
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #4084 +/- ##
==========================================
- Coverage 84.40% 84.07% -0.33%
==========================================
Files 570 570
Lines 53071 53099 +28
Branches 3054 3059 +5
==========================================
- Hits 44794 44643 -151
- Misses 7318 7494 +176
- Partials 959 962 +3 ☔ View full report in Codecov by Sentry. |
b9d8499
to
24e1167
Compare
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
deepmd/dpmodel/descriptor/dpa1.py
Outdated
variables = data.pop("@variables") | ||
embeddings = data.pop("embeddings") | ||
attention_layers = data.pop("attention_layers") | ||
env_mat = data.pop("env_mat") |
Check notice
Code scanning / CodeQL
Unused local variable Note
data.pop("type") | ||
variables = data.pop("@variables") | ||
embeddings = data.pop("embeddings") | ||
env_mat = data.pop("env_mat") |
Check notice
Code scanning / CodeQL
Unused local variable Note
deepmd/dpmodel/utils/seed.py
Outdated
@@ -12,10 +11,10 @@ | |||
|
|||
|
|||
@overload | |||
def child_seed(seed: Union[int, List[int]], idx: int) -> List[int]: ... | |||
def child_seed(seed: Union[int, list[int]], idx: int) -> list[int]: ... |
Check notice
Code scanning / CodeQL
Statement has no effect Note
deepmd/infer/deep_property.py
Outdated
def eval( | ||
self, | ||
coords: np.ndarray, | ||
cells: Optional[np.ndarray], | ||
atom_types: Union[list[int], np.ndarray], | ||
atomic: bool = False, | ||
fparam: Optional[np.ndarray] = None, | ||
aparam: Optional[np.ndarray] = None, | ||
mixed_type: bool = False, | ||
**kwargs: dict[str, Any], | ||
) -> tuple[np.ndarray, ...]: |
Check notice
Code scanning / CodeQL
Returning tuples with varying lengths Note
tuple of size 1
tuple of size 2
deepmd/infer/model_devi.py
Outdated
@@ -29,7 +28,7 @@ | |||
real_f: Optional[np.ndarray] = None, | |||
relative: Optional[float] = None, | |||
atomic: Literal[False] = ..., | |||
) -> Tuple[np.ndarray, np.ndarray, np.ndarray]: ... | |||
) -> tuple[np.ndarray, np.ndarray, np.ndarray]: ... |
Check notice
Code scanning / CodeQL
Statement has no effect Note
deepmd/jax/common.py
Outdated
|
||
|
||
@overload | ||
def to_jax_array(array: np.ndarray) -> jnp.ndarray: ... |
Check notice
Code scanning / CodeQL
Statement has no effect Note
deepmd/jax/common.py
Outdated
|
||
|
||
@overload | ||
def to_jax_array(array: None) -> None: ... |
Check notice
Code scanning / CodeQL
Statement has no effect Note
deepmd/jax/infer/deep_eval.py
Outdated
) | ||
|
||
if TYPE_CHECKING: | ||
import ase.neighborlist |
Check notice
Code scanning / CodeQL
Unused import Note
deepmd/jax/utils/serialization.py
Outdated
) | ||
elif model_file.endswith(".hlo"): | ||
model = BaseModel.deserialize(data["model"]) | ||
model_def_script = data["model_def_script"] |
Check notice
Code scanning / CodeQL
Unused local variable Note
deepmd/pt/model/descriptor/dpa2.py
Outdated
if data["repinit"].use_three_body: | ||
# deserialize repinit_three_body | ||
statistic_repinit_three_body = repinit_three_body_variable.pop("@variables") | ||
env_mat = repinit_three_body_variable.pop("env_mat") |
Check warning
Code scanning / CodeQL
Variable defined multiple times Warning
redefined
for more information, see https://pre-commit.ci
Thie PR aims at fixing #3679.
The
get_valid_pairs
will raise an error whenThe origin is that the check in
get_valid_pairs
is too strict for the initialization process. The check requires that a pair of real and virtual atoms found by iterating over the bondlist to be the local atoms of the same processor. This requirement is automatically satisfied during the running of MD because the positions of virtual atoms are set equal to those of real atoms before rebuilding bondlists. However, during initialization (Verlet::setup(int flag)
), the bondlist is built based on the initial configuration provided by the user. If the user provides a configuration in which the virtual atoms are placed at the Wannier centroids, it is possible that a bond formed by a pair of real and virtual atoms are between a local and a ghost atom, which triggers this error.This PR loosens the requirement on the initial configuration provided by the user. In the
setup_post_neighbor
function, theget_valid_pairs
function searches for bonded pairs but skips the aforementioned check. It sets the coordinates of the virtual atoms to the coordinates of the real atoms, and then rebuilds the bondlist. Afterwards, the check can be passed.Summary by CodeRabbit
post_integrate
method topre_exchange
to better reflect its role in the simulation process.