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

[Feature Request] add unit tests #838

Closed
43 of 61 tasks
njzjz opened this issue Jul 9, 2021 · 5 comments
Closed
43 of 61 tasks

[Feature Request] add unit tests #838

njzjz opened this issue Jul 9, 2021 · 5 comments

Comments

@njzjz
Copy link
Member

njzjz commented Jul 9, 2021

Summary

Add unit tests for the following methods or classes.

Detailed Description

@shishaochen
Copy link
Collaborator

shishaochen commented Aug 1, 2021

While improvement comes in #913, these 3 functions seems have problem in unit tests for 100% code coverage.

  • deepmd.cluster.slurm._pad_zeros
  • deepmd.cluster.slurm._expand_ids
  • deepmd.cluster.slurm._expand_nodelist

One reason is that both _expand_nodelist and _expand_ids use commas to separate tokens. Considering execution order, function _expand_ids has no chance to handle argument ids with any comma. Consequently, function _pad_zeros will never be called.

@denghuilu Do you have any document on the schema of environment variable SLURM_JOB_NODELIST? Maybe we can write a better parsing logic. All I can find is Slurm Environmental Variables and slurm.h.in.

@denghuilu
Copy link
Member

While improvement comes in #913, these 3 functions seems have problem in unit tests for 100% code coverage.

  • deepmd.cluster.slurm._pad_zeros
  • deepmd.cluster.slurm._expand_ids
  • deepmd.cluster.slurm._expand_nodelist

One reason is that both _expand_nodelist and _expand_ids use commas to separate tokens. Considering execution order, function _expand_ids has no chance to handle argument ids with any comma. Consequently, function _pad_zeros will never be called.

@denghuilu Do you have any document on the schema of environment variable SLURM_JOB_NODELIST? Maybe we can write a better parsing logic. All I can find is Slurm Environmental Variables and slurm.h.in.

I also found the possible definition at https://hpcc.umd.edu/hpcc/help/slurmenv.html. @amcadmus Any suggestions?

@njzjz
Copy link
Member Author

njzjz commented Aug 2, 2021

See here: https://slurm.schedmd.com/faq.html

@shishaochen
Copy link
Collaborator

shishaochen commented Aug 2, 2021

@njzjz @denghuilu The tiny package python-hostlist seems professional.

Now code coverages on both deepmd.cluster.local and deepmd.cluster.slurm reach 100% with help of #913.

njzjz pushed a commit to njzjz/deepmd-kit that referenced this issue Sep 21, 2023
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.

Co-authored-by: HuangJiameng <you@huangjm@dp.tech>
@njzjz
Copy link
Member Author

njzjz commented Oct 18, 2023

Close since most of the tasks have been done.

@njzjz njzjz closed this as completed Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants