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 #77

Merged
merged 34 commits into from
Oct 12, 2019
Merged

Ase interface #77

merged 34 commits into from
Oct 12, 2019

Conversation

YuuuXie
Copy link
Collaborator

@YuuuXie YuuuXie commented Sep 27, 2019

  1. add ase folder, and remove ase*.py in flare folder
  2. ase module includes flare calculator, otf engine and logger
  3. different md engines are tested with otf (src: otf_md.py, unit test: test_ase_setup/test_ase_otf.py with QE)
  4. add .rst for mgp and ase in docs folder
  5. cleaned up *.out in tests folder since they are not used

res.append(predict_on_atom((structure, atom, gp_model)))
return res

def predict_on_structure_en(strucutre, gp_model):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Input should be "structure"


self.mgp_model = MappedGaussianProcess(self.gp_model, grid_params, struc_params)

def predict_on_atom(params):
Copy link
Collaborator

Choose a reason for hiding this comment

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

These functions look pretty similar to the ones in predict.py, would it be possible to use those so that we don't repeat code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea. I'll make the change

@@ -18,7 +17,7 @@
qe_input_to_structure, parse_qe_forces
from flare import output

class MFFOTF(OTF):
class MGPOTF(OTF):
Copy link
Collaborator

Choose a reason for hiding this comment

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

We now have three otf.py files in the repo -- mgp/otf, which inherits from flare/otf, and ase/otf, which is separate. If there are common methods to all three, we should try to import them from one place to minimize duplication.

@YuuuXie
Copy link
Collaborator Author

YuuuXie commented Sep 27, 2019

Jon: We now have three otf.py files in the repo -- mgp/otf, which inherits from flare/otf, and ase/otf, which is separate. If there are common methods to all three, we should try to import them from one place to minimize duplication.

@YuuuXie
Copy link
Collaborator Author

YuuuXie commented Sep 27, 2019

One common method that can be extracted out is is_std_in_bound which is sort of independent. MGPOTF is short because it inherits most of the methods from OTF. ase/otf is coupled with ase calculator, ase logger and ase md engines, so will be harder to be put out

@jonpvandermause
Copy link
Collaborator

Yeah, is_std_in_bound would be good to have in one place (#30). We can either move it out of class in otf.py, or put it in predict.py.

@YuuuXie
Copy link
Collaborator Author

YuuuXie commented Sep 27, 2019

I can also have ase/otf to inherit from OTF somehow, since there're a lot of common attributes. Could we probably put is_std_in_bound in util.py? I think moving it in predict.py is a bit weird

Copy link
Collaborator Author

@YuuuXie YuuuXie left a comment

Choose a reason for hiding this comment

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

Oh I used to use the same dir as you do (../test_files/pseudos) and never passed TRAVIS test. Then Lixin figured out TRAVIS did its unit test in the tests folder, so it runs python test_ase_setup/test_ase_otf.py. That's why I changed it to test_files/pseudos and then it passed unit test


self.mgp_model = MappedGaussianProcess(self.gp_model, grid_params, struc_params)

def predict_on_atom(params):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea. I'll make the change

@jonpvandermause
Copy link
Collaborator

Oh I used to use the same dir as you do (../test_files/pseudos) and never passed TRAVIS test. Then Lixin figured out TRAVIS did its unit test in the tests folder, so it runs python test_ase_setup/test_ase_otf.py. That's why I changed it to test_files/pseudos and then it passed unit test

Good catch. The test failed on my machine, but that's because I was running a different command than Travis. I switched it back.

@codecov-io
Copy link

codecov-io commented Sep 28, 2019

Codecov Report

Merging #77 into master will increase coverage by 10.77%.
The diff coverage is 82.65%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #77       +/-   ##
===========================================
+ Coverage   41.19%   51.97%   +10.77%     
===========================================
  Files          29       29               
  Lines        4954     4839      -115     
===========================================
+ Hits         2041     2515      +474     
+ Misses       2913     2324      -589
Impacted Files Coverage Δ
flare/mgp/otf.py 0% <0%> (ø) ⬆️
flare/mgp/cubic_splines_numba.py 5.87% <100%> (ø) ⬆️
flare/mgp/splines_methods.py 62.23% <100%> (+12.58%) ⬆️
flare/struc.py 97.16% <100%> (+0.16%) ⬆️
flare/mgp/mgp.py 98.17% <100%> (+2.76%) ⬆️
flare/otf.py 95.12% <100%> (+1.37%) ⬆️
flare/ase/logger.py 73.28% <73.28%> (ø)
flare/ase/otf.py 73.68% <73.68%> (ø)
flare/ase/calculator.py 92.85% <92.85%> (ø)
flare/util.py 91.11% <93.33%> (+1.11%) ⬆️
... and 14 more

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 840055d...5a7da10. Read the comment docs.

@anjohan
Copy link
Collaborator

anjohan commented Sep 29, 2019

Re: Travis testing directory.

I set Travis to run its tests from the tests directory. When I try to run pytest from the base directory, I get a lot of errors from trying to locate ./test_files/cp2k_input_1.in etc.

@YuuuXie
Copy link
Collaborator Author

YuuuXie commented Sep 29, 2019

Re: Travis testing directory.

I set Travis to run its tests from the tests directory. When I try to run pytest from the base directory, I get a lot of errors from trying to locate ./test_files/cp2k_input_1.in etc.

I also tried running pytest ./ in the tests/ folder, but it works

@YuuuXie
Copy link
Collaborator Author

YuuuXie commented Sep 29, 2019

Yeah, is_std_in_bound would be good to have in one place (#30). We can either move it out of class in otf.py, or put it in predict.py.

The latest commits removed the predict functions in ase/otf.py, and uses predict.py. Also removed is_std_in_bound in otf.py and ase/otf.py, and moved it to util.py

This pull request can close #26 , #30 , #55

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.

5 participants