-
Notifications
You must be signed in to change notification settings - Fork 71
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
Better user interface and consistency check through out env/gp/kernel modules #184
Conversation
… into user_interface_refactory Conflicts: flare/mgp/mgp.py
…flare into user_interface_refactory Conflicts: flare/env.py flare/mgp/mgp.py tests/test_env.py
… into user_interface_refactory
… into user_interface_refactory
…ce spc_set with 'sort'
if std_in_bound: | ||
new_pos = self.md_step() | ||
|
||
else: | ||
# record GP forces | ||
self.update_temperature(new_pos) |
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.
The logic is wrong here; velocities get sent to zero, since new_pos hasn't been updated. (I'll fix this)
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.
8544b9f addresses this by mirroring the otf loop on master (new positions are calculated first, and then is_std_in_bound is called)
flare/ase/otf.py
Outdated
FLARE_Calculator or DFT calculator | ||
''' | ||
self.md.step() | ||
return self.atoms.positions |
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.
This is different from the md_step method in otf.py, which computes the new positions but doesn't overwrite the positions attribute of the structure
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've attempted to resolve this with 125c08f by removing the second call to md.step() in the otf loop
Get new position in molecular dynamics based on the forces predicted by | ||
FLARE_Calculator or DFT calculator | ||
''' | ||
self.md.step() |
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.
Somehow, self.md.step() updates the positions of self.structure, even though it should act only on self.atoms. This causes the velocities and temperatures to be reported as zero in the OTF output file.
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.
1829b0c seems to give a temporary fix to this problem by manually resetting the positions of self.structure. But I'm still confused why self.md.step() updates self.structure.positions...
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.
The issue was that self.md.step() returned atoms.positions rather than a copy of it, which caused self.atoms and self.structure to get updated together downstream. a2a785a fixes this
…up/flare into user_interface_refactory
… into user_interface_refactory
kernels (list, optional): Determine the type of kernels. Example: | ||
['2', '3'], ['2', '3', 'mb'], ['2']. Defaults to ['2', '3'] |
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.
This was a nice change.
@@ -682,33 +625,29 @@ def as_dict(self): | |||
|
|||
return out_dict | |||
|
|||
def sync_data(self): |
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.
Maybe we should move the setting of the NP training labels to this function, so that it's only a matter of adding to training data and training labels?
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.
Updating the 'all labels' attribute could also happen here.
kernel, grad, ek, efk = str_to_kernel_set(kernel_name, multihyps) | ||
def update_kernel(self, kernels, component="mc", hyps_mask=None): | ||
kernel, grad, ek, efk = str_to_kernel_set( | ||
kernels, component, hyps_mask) |
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.
No docstring & typehints? :)
@@ -962,3 +933,76 @@ def __del__(self): | |||
if (self.name in _global_training_labels): | |||
_global_training_labels.pop(self.name, None) | |||
_global_training_data.pop(self.name, None) | |||
|
|||
@staticmethod | |||
def backward_arguments(kwargs, new_args={}): |
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.
Love these deprecation warnings. It's great we have this! Will save a lot of grief in using old scripts
@@ -74,7 +75,7 @@ def __init__(self, frames: List[Structure], | |||
max_trains: int = np.inf, | |||
min_atoms_per_train: int = 1, | |||
shuffle_frames: bool = False, | |||
verbose: int = 1, | |||
verbose: str = "INFO", |
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.
Great call to re-do it this way
flare/mgp/utils.py
Outdated
# triplet1 = array([r1, r2, r12]) | ||
# triplet2 = array([r2, r1, r12]) | ||
# | ||
# if spc1 <= spc2: | ||
# spcs = [ctype, spc1, spc2] | ||
# else: | ||
# spcs = [ctype, spc2, spc1] | ||
# | ||
# triplet = [triplet1, triplet2] | ||
# coord = [c1, c2] | ||
# | ||
# if spcs not in exist_species: | ||
# exist_species.append(spcs) | ||
# tris += [triplet] | ||
# tri_dir += [coord] | ||
# else: | ||
# k = exist_species.index(spcs) | ||
# tris[k] += triplet | ||
# tri_dir[k] += coord |
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.
Any reason why these guys are still here?
print('running test_otf.py') | ||
print('current working directory:') | ||
print(os.getcwd()) |
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 suspect these were accidentally left over from debugging?
print('running test_otf.py') | ||
print('current working directory:') | ||
print(os.getcwd()) |
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.
Likewise :)
LGTM (looks good to me). Major kudos to @nw13slx and @YuuuuXie for all their blood, sweat, and tears in making FLARE easier to use and maintain. This is really important work that often goes underappreciated. |
Major changes:
Minor changes:
An example of the new GP initialization: