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

wfl's QE calculator with directory handling. #136

Merged
merged 21 commits into from
Aug 18, 2022
Merged

wfl's QE calculator with directory handling. #136

merged 21 commits into from
Aug 18, 2022

Conversation

gelzinyte
Copy link
Contributor

Added a Calculator which extends ASE's espresso calculator to execute each calculation in a separate directory and handle parameters in awfl.caclulators.espresso.evaluate_autopara_wrappable way. Workflow's espresso calculator now can be parallelised with the generic.py.

@bernstei once I have taken into account your comments, I'd like to remove the dft.py & espresso.evaluate_autopara_wrappable QE interface and rewrite VASP and CASTEP in a similar way.

@bernstei
Copy link
Contributor

@bernstei once I have taken into account your comments, I'd like to remove the dft.py & espresso.evaluate_autopara_wrappable QE interface and rewrite VASP and CASTEP in a similar way.

Do you mean comments I made earlier, or comments about this particular set of changes?

@gelzinyte
Copy link
Contributor Author

I meant these changes in particular. Though on the second thought, it makes more sense to review everything in one go, so I have removed the previous QE interface.

About previous comments - I didn't implement reuse of run directories (you mentioned it in #17), I would leave that for another issue/PR. And I don't think there is anything that could be conveniently generalised for other wfl's calculators like this, but let me know what you think.

@bernstei
Copy link
Contributor

I'll look over the changes, but do you know why the QE test is failing? The numbers are close, but not within the tolerance.

@gelzinyte
Copy link
Contributor Author

The reference values were what I got on our cluster, but the values on github are different. I am updating the tolerance requirements, although I'm not sure if that's the best thing to do.

@bernstei
Copy link
Contributor

The reference values were what I got on our cluster, but the values on github are different. I am updating the tolerance requirements, although I'm not sure if that's the best thing to do.

Forces are generally harder to converge than energies, and the specific values it'll converge to are likely to depend on architecture, number of cores, etc, so I'm not so surprised the tolerance can't be super tight.

@bernstei
Copy link
Contributor

bernstei commented Aug 17, 2022

[edited] I guess the idea is that the calculate() itself calls setup_params_for_this_calc which makes sense. Let me think some more.

@bernstei
Copy link
Contributor

[edited] I guess the idea is that the calculate itself calls setup_params_for_this_calc which makes sense. Let me think some more.

OK - I looked at generic.py more carefully, and I think the warning is fine (may not even be needed, but let's leave it for now). I'm updating the docstring.

@bernstei
Copy link
Contributor

I just pushed a little fix to the import in dft.py. If it passes, I think we can merge.

@bernstei
Copy link
Contributor

@gelzinyte do you think we should merge now?

@gelzinyte
Copy link
Contributor Author

Thanks for looking through - do merge if the last unresolved conversation is ok with you.

@bernstei bernstei merged commit 18052f5 into main Aug 18, 2022
@gelzinyte gelzinyte deleted the qe-update branch August 30, 2022 13:40
@gelzinyte gelzinyte mentioned this pull request Oct 14, 2022
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.

2 participants