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

support merge_traj for lammps #838

Merged
merged 12 commits into from
Aug 30, 2022

Conversation

HuangJiameng
Copy link
Collaborator

Feature Request sees #584. Add new parameter model_devi_merge_traj to solve this problem. If model_devi_merge_traj is set as True, only all.lammpstrj will be generated, instead of lots of small traj files. Unittest will be added later by @Chengqian-Zhang. Only LAMMPS is supported up to now.

@codecov-commenter
Copy link

codecov-commenter commented Aug 3, 2022

Codecov Report

Merging #838 (14dd401) into devel (5a57de2) will increase coverage by 2.99%.
The diff coverage is 82.97%.

@@            Coverage Diff             @@
##            devel     #838      +/-   ##
==========================================
+ Coverage   35.16%   38.15%   +2.99%     
==========================================
  Files          96       99       +3     
  Lines       16817    17875    +1058     
==========================================
+ Hits         5913     6820     +907     
- Misses      10904    11055     +151     
Impacted Files Coverage Δ
dpgen/generator/run.py 61.92% <73.46%> (-1.78%) ⬇️
dpgen/generator/lib/lammps.py 61.07% <90.90%> (-11.52%) ⬇️
dpgen/generator/arginfo.py 100.00% <100.00%> (+50.86%) ⬆️
dpgen/generator/lib/make_calypso.py 70.06% <0.00%> (-7.46%) ⬇️
dpgen/dispatcher/AWS.py 25.26% <0.00%> (-2.87%) ⬇️
dpgen/data/gen.py 47.88% <0.00%> (-0.60%) ⬇️
dpgen/tools/relabel.py 14.60% <0.00%> (-0.14%) ⬇️
dpgen/generator/lib/run_calypso.py 10.00% <0.00%> (ø)
... and 25 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

dpgen/generator/arginfo.py Outdated Show resolved Hide resolved
dpgen/generator/lib/lammps.py Outdated Show resolved Hide resolved
dpgen/generator/lib/lammps.py Outdated Show resolved Hide resolved
dpgen/generator/run.py Outdated Show resolved Hide resolved
@njzjz
Copy link
Member

njzjz commented Aug 18, 2022

Have you successfully run it? If you run it you should find the error..

@HuangJiameng
Copy link
Collaborator Author

Have you successfully run it? If you run it you should find the error..

Yes, I have successfully run it. It's weird... I will test it again.

@Chengqian-Zhang
Copy link
Collaborator

I have tested the new function and the traj files have been actually merged to a single file . However , I find that the traj folder still exists and it is empty. And the unittest has some error , I have sent the log to you.

@Chengqian-Zhang
Copy link
Collaborator

I tested again and found that the traj empty folder is gone and the unit tests are fine

@wanghan-iapcm wanghan-iapcm requested a review from njzjz August 26, 2022 02:51
@njzjz
Copy link
Member

njzjz commented Aug 26, 2022

I tested again and found that the traj empty folder is gone and the unit tests are fine

Where are the unit tests?

@Chengqian-Zhang
Copy link
Collaborator

I tested again and found that the traj empty folder is gone and the unit tests are fine

Where are the unit tests?

OK...I found that the unit tests she gave me was not changed, I will raise another pr to modify the unit tests after this pr is merged.

@@ -167,6 +171,42 @@ def get_dumped_forces(
ret = np.array(ret)
return ret

def get_all_dumped_forces(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A UT for this function should be provided.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Chengqian-Zhang You can directly pull request to HuangJiameng:merge_traj branch, and then this PR will be updated.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wanghan-iapcm @AnguseZhang @njzjz The UT for this function has been provided.Please check.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no problems on this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] model_devi trajectory data keep in a single files.
6 participants