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

Ase interface - close #133 #136

Merged
merged 5 commits into from
Feb 13, 2020
Merged

Ase interface - close #133 #136

merged 5 commits into from
Feb 13, 2020

Conversation

YuuuXie
Copy link
Collaborator

@YuuuXie YuuuXie commented Feb 4, 2020

fixed the ASE logger issue reported in #133

def get_property(self, atoms, property_name):
if property_name not in self.results.keys():
def get_property(self, name, atoms=None, allow_calculation=True):
if name not in self.results.keys():
Copy link
Contributor

@smheidrich smheidrich Feb 5, 2020

Choose a reason for hiding this comment

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

This PR seems to fix my issue (thanks!) but makes the code run insanely slowly compared to having trajectory=None in my tests. I think allow_calculation should be actually handled here the way ASE does it:

Suggested change
if name not in self.results.keys():
if name not in self.results.keys():
if not allow_calculation:
return None

⇒ Fast again

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for finding this issue and giving the suggestion. A new commit has been made on this allow_calculation change.

@codecov-io
Copy link

codecov-io commented Feb 5, 2020

Codecov Report

Merging #136 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #136      +/-   ##
=========================================
+ Coverage   59.29%   59.3%   +0.01%     
=========================================
  Files          31      31              
  Lines        4761    4760       -1     
=========================================
  Hits         2823    2823              
+ Misses       1938    1937       -1
Impacted Files Coverage Δ
flare/ase/logger.py 95% <ø> (ø) ⬆️
flare/ase/calculator.py 81.72% <100%> (+0.4%) ⬆️
flare/ase/otf.py 60.34% <100%> (-0.11%) ⬇️
flare/ase/otf_md.py 98.14% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7afbd4b...6615cfa. Read the comment docs.

@smheidrich
Copy link
Contributor

Thanks again for fixing this, I think there is one more issue with this PR: The resulting trajectory file has about twice as many frames as there were MD steps - I guess this is because, as far as I understand, call_observers is called a second time whenever there is a DFT calculation, whereas in ASE, it's called only once per MD step.

@YuuuXie
Copy link
Collaborator Author

YuuuXie commented Feb 6, 2020

Thanks for pointing out this. Actually we were not using ASE trajectory for doing things, so we did not consider the compatibility with it. Since we have our own observer/logger, we have to record the positions and forces from DFT calculations, so as to collect training data, that's why call_observers is also used in DFT call.

I will open a new issue for this enhancement, i.e. to make our log file better compatible with ASE primitive log files, or even merge some of them.

For your ASE trajectory file, a temporary solution is to remove the DFT frames in post processing. You can look at the folder otf_data generated by your otf run. In the file dft_positions.xyz all the frames for DFT calculation are collected, you will see lines like Frame: x meaning at step x, GP gives a force prediction, and then DFT is also called to make a force prediction for the same frame.

For example, in the toy example you gave (in #133 ), you can run the following to remove the DFT frames in your trajectory file (the new_trj is the new trajectory file with DFT frames removed):

from ase.io.trajectory import Trajectory

# extract dft frame number from file
dft_file = open('otf_data/dft_positions.xyz')
dft_frames = []
for line in dft_file.readlines():
    if 'Frame' in line:
        dft_frames.append(int(line.split()[-1]))

# ase trajectory
trj = Trajectory('otf_md.traj')
new_trj = Trajectory('otf_md_new.traj', mode='w')
j = 0
for i in range(len(trj)):
    md_step = len(new_trj)
    if md_step == dft_frames[j]: # the current step is dft 
        j += 1
        continue
    else:
        new_trj.write(trj[i])

Let me know if you find any other issues

@smheidrich
Copy link
Contributor

smheidrich commented Feb 6, 2020

@YuuuuXie Thanks a lot for going to the trouble of writing a workaround! I'll use this for the time being.

Making FLARE's OTF-MD stuff behave more like ASE's MD classes in general would be great, too, of course.

@nw13slx nw13slx merged commit 6f0dd93 into master Feb 13, 2020
@nw13slx nw13slx deleted the ase_interface branch February 13, 2020 22:09
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.

4 participants