Enable setting test size individually for each system#267
Merged
amcadmus merged 7 commits intodeepmodeling:develfrom Oct 19, 2020
Merged
Enable setting test size individually for each system#267amcadmus merged 7 commits intodeepmodeling:develfrom
amcadmus merged 7 commits intodeepmodeling:develfrom
Conversation
njzjz
requested changes
Oct 7, 2020
Member
There was a problem hiding this comment.
The unittest was not passing here:
deepmd-kit/source/tests/test_deepmd_data_sys.py
Lines 79 to 100 in 59d780c
Contributor
Author
|
I am taking a look at it, just haven't figured out where the problem is yet. |
Contributor
Author
|
Everything should be in order now. |
Member
|
The new features of the code will be merge to devel branch. |
njzjz
approved these changes
Oct 9, 2020
amcadmus
requested changes
Oct 16, 2020
clear the confusion caused by adding python style comments to json file
njzjz
requested changes
Oct 17, 2020
Co-authored-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Co-authored-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
amcadmus
approved these changes
Oct 19, 2020
gzq942560379
pushed a commit
to HPC-AI-Team/deepmd-kit
that referenced
this pull request
Sep 1, 2021
Enable setting test size individually for each system
njzjz
pushed a commit
to njzjz/deepmd-kit
that referenced
this pull request
Sep 21, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When one is dealing with multiple systems which have very distinct numbers of frames setting number of tests as only one integer which is same for all systems is rather restraining. For example consider two systems:
Setting number of tests to 10 should be ideal for the second system but is far too low for the first one. There is no good solution to this situation, since increasing test number to say 100 would be appropriate for the first system but wrong for the second.
The change I propose simply allows to set
numb_testparameter as a list with separate value for each system or as a string specifying a percentage of the systems frames that should be used for testing.In the first case, number of tests could be specified in json as
numb_test : [1000, 10]or"10%"which would amount to 10% of testing frames for each of the systems respectively in both of the settings.This requires addition of only a few lies of code and minimal new logic, all changes are commented in the code.
I am pushing this to devel branch, since I don't know your workflow, but if you accept it can be merged directly to master too as it would be nice to see it in conda and docker as soon as possible.
I also made a few minor changes to README which I think serve to better clarify the purpose of json settings.