Skip to content
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(cc): fix message passing when nloc is 0 #4021

Merged
merged 20 commits into from
Jul 26, 2024

Conversation

njzjz
Copy link
Member

@njzjz njzjz commented Jul 25, 2024

Fix #3732. Removes the special codes for nloc==0 which has been supported by #4005.

Summary by CodeRabbit

  • New Features
    • Improved robustness in tensor handling when inputs are empty.
    • Enhanced test coverage for parameters in the test_pair_deepmd_si and test_pair_deepmd_mpi functions, testing scenarios with and without balance arguments.
  • Bug Fixes
    • Adjusted control flow in the compute method to simplify handling of zero atoms or local atoms.

njzjz added 20 commits June 26, 2024 17:01
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
This reverts commit 546c798.
This reverts commit 51a94b6.
This reverts commit e264828.
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
This reverts commit 3ac3ce4.
This reverts commit 6e82ca4.
Copy link
Contributor

coderabbitai bot commented Jul 25, 2024

Walkthrough

Walkthrough

The updates enhance the DeepPotPT class's tensor management and modify the control flow in the compute method, particularly addressing scenarios involving empty input tensors. Additionally, the test coverage for the test_pair_deepmd_si function has been expanded to include tests with both the --balance argument and without it, improving the robustness of the testing process.

Changes

Files Change Summary
source/api_cc/src/DeepPotPT.cc Modified createNlistTensor to handle empty row_tensors, ensuring robustness. Adjusted compute method by removing logic for zero atoms, altering control flow.
source/lmp/tests/test_lammps_dpa_pt.py
source/lmp/tests/test_lammps_dpa_sel_pt.py
Updated test_pair_deepmd_si function to include an additional test case with an empty list, increasing test coverage.

Assessment against linked issues

Objective Addressed Explanation
dpa2 model may need a fake communication op to deal with nloc == 0. (#3732) Unclear if changes address communication needs for nloc == 0.

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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@njzjz njzjz changed the title fix: fix message passing when nloc is 0 fix(cc): fix message passing when nloc is 0 Jul 25, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 (1)
source/api_cc/src/DeepPotPT.cc (1)

Verify behavior for zero atoms or local atoms.

The compute method does not explicitly handle cases where natoms or nloc are zero. This could lead to potential issues during runtime. Please review and ensure that scenarios with zero atoms or local atoms are handled correctly.

  • File: source/api_cc/src/DeepPotPT.cc
  • Lines: 38-49
Analysis chain

Line range hint 110-110:
Verify behavior for zero atoms or local atoms.

The removal of blocks handling cases where nloc or natoms are zero simplifies the control flow but may affect the method's behavior in edge cases. Ensure that the method handles scenarios with zero atoms or local atoms correctly.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the behavior of the `compute` method for zero atoms or local atoms.

# Test: Search for the method usage and check for edge cases handling.
rg --type cpp -A 10 $'DeepPotPT::compute'

Length of output: 10872


Script:

#!/bin/bash
# Description: Retrieve the complete implementation of the `compute` method to verify handling of zero atoms or local atoms.

# Search for the complete implementation of the `compute` method.
ast-grep --lang cpp --pattern $'void DeepPotPT::compute($_, $$$) {
  $$$
}'

Length of output: 76


Script:

#!/bin/bash
# Description: Retrieve the complete implementation of the `compute` method to verify handling of zero atoms or local atoms.

# Search for the `compute` method definition and include 50 context lines to capture the full implementation.
rg --type cpp -A 50 'void DeepPotPT::compute'

Length of output: 20194


Script:

#!/bin/bash
# Description: Retrieve the complete implementation of the `compute` method to verify handling of zero atoms or local atoms.

# Search for the `compute` method definition and include 100 context lines to capture the full implementation.
rg --type cpp -A 100 'void DeepPotPT::compute'

Length of output: 27505

Copy link

codecov bot commented Jul 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.94%. Comparing base (5dd0062) to head (ae70351).
Report is 109 commits behind head on devel.

Additional details and impacted files
@@            Coverage Diff             @@
##            devel    #4021      +/-   ##
==========================================
+ Coverage   82.92%   82.94%   +0.01%     
==========================================
  Files         522      522              
  Lines       51010    50991      -19     
  Branches     3023     3022       -1     
==========================================
- Hits        42301    42294       -7     
+ Misses       7767     7757      -10     
+ Partials      942      940       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wanghan-iapcm wanghan-iapcm added this pull request to the merge queue Jul 26, 2024
Merged via the queue into deepmodeling:devel with commit 7907142 Jul 26, 2024
60 checks passed
mtaillefumier pushed a commit to mtaillefumier/deepmd-kit that referenced this pull request Sep 18, 2024
Fix deepmodeling#3732. Removes the special codes for `nloc==0` which has been
supported by deepmodeling#4005.

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

- **New Features**
	- Improved robustness in tensor handling when inputs are empty.
- Enhanced test coverage for parameters in the `test_pair_deepmd_si` and
`test_pair_deepmd_mpi` functions, testing scenarios with and without
balance arguments.
- **Bug Fixes**
- Adjusted control flow in the `compute` method to simplify handling of
zero atoms or local atoms.


<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dpa2 model may need a fake communication op to deal with nloc == 0.
4 participants