-
Notifications
You must be signed in to change notification settings - Fork 68
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
Added GPFA parser function to gp_from_aimd #173
Conversation
Codecov Report
@@ Coverage Diff @@
## master #173 +/- ##
==========================================
+ Coverage 58.10% 58.47% +0.36%
==========================================
Files 35 35
Lines 7467 7533 +66
==========================================
+ Hits 4339 4405 +66
Misses 3128 3128
Continue to review full report at Codecov.
|
@@ -182,7 +182,7 @@ def __init__(self, frames: List[Structure], | |||
assert (isinstance(skip, int) and skip >= 1), "Skip needs to be a " \ | |||
"positive integer." | |||
self.validate_ratio = validate_ratio | |||
assert (validate_ratio >= 0 and validate_ratio <= 1), \ | |||
assert (0 <= validate_ratio <= 1), \ |
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.
Didn't know that python can do this. Cool
@@ -243,11 +243,16 @@ def pre_run(self): | |||
self.gp.opt_algorithm, | |||
dt=0, | |||
Nsteps=len(self.frames), | |||
structure=self.frames[0], |
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.
It's good to remove this. It gave me some trouble as well...
:return: | ||
""" | ||
frames, gp_data = parse_trajectory_trainer_output( | ||
'./test_files/gpfa_parse_test.out', True) |
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.
Is it possible to also use the gpfa unit test output as input here? so the test can check whether the code is self consistent?
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.
That's a nice idea and it occurred to me; gpfa_parse_test.out
is based on a unit test output, but I also wanted to make sure the tests could run independently. Is there a better way to do this you see?
|
||
initial_gp_statistics = json.loads(gp_stats_line) | ||
|
||
# Get pre_run statistics (if pre-run was done): |
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.
do we have a mechanism to check whether the gp model began with some training data before the whole gpfa run? ideally, we can throw a warning to let the user know that the parser will not fully recovered the gp model unless they use the same initial model.
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.
I never know where to set the bar with these things, but, I think it might be okay to trust the user here. The way I see it, the initial GP statistics being empty or not is the signal to the user. However-- I think this is a great idea to keep in our back pocket if/when we write a `rewind GPFA' function, and the warning should be raised then that the GPFA output is not a complete record of the GP's training.
Adds:
parse_trajectory_trainer_output
togp_from_aimd.py
which allows for easy parsing on both a frame-by-frame level as well as on the GP level.A unit test for the parser, and a test file for it to run on.
Changes:
Certain
output.py
functions now have their arguments changed to allow optional input. This should change nothing about the way they are used in other scripts, but makes for cleaner GPFA output parsing.Minor cosmetic change to GaussianProcess' training_statistics method.