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

Add espresso recipes and custom calculator #1238

Merged
merged 157 commits into from
Dec 11, 2023

Conversation

tomdemeyere
Copy link
Contributor

@tomdemeyere tomdemeyere commented Nov 22, 2023

  • I have read the Developer Guide. Don't lie! 😉
  • My PR is on a custom branch and is not named main.
  • I have added relevant unit tests. Note: Your PR will likely not be merged without proper tests.

A first PR for espresso additions, this is very much draft stage and does not aim to work, just to show the general idea. Right now I have other things to do but I believe this is a good first draft. There will be probably a thousand details that you don't like in this PR (variable names, etc...) so I will let you have a look. I will come back to it later on.

The aim is to make it simple for the future when adding other espresso binaries, for that I created mini-classes based on a more generic 'EspressoCalculator' class. The process to add a binary would be as follows:

  1. Add a class in quacc.calculators.espresso.calculator (few lines)
  2. Add a Fortran keylist in quacc.calculators.espresso.keys
  3. add a write_BIN_specific function into quacc.calculators.espresso.io as well as read_bin_output

And that's it, there is nothing else to change, everything is done under the hood. Stage 3 is by far the most time-consuming, 1 and 2 shouldn't take more than a few minutes of work, and GPT is able to provide 2 if needed based on the binary doc page.

Presets are a good idea, I added a quick one based on the cutoff table given by the SSSP library. I am thinking about adding more specific ones in the future (slabs, bulk...)

I think for the pp we shouldn't deal with env variables other than the QUACC ones since I am not aware of how different workflow managers deal with them. Covalent for example always uses env variables defined when the server starts and it does not seem intuitive to change them on the fly.

ASR edit: Closes #1224. Closes #1306. Closes #1307.

@Andrew-S-Rosen
Copy link
Member

@tomdemeyere: Fantastic! Thank you very much. I look forward to giving this a look shortly.

Copy link
Member

@Andrew-S-Rosen Andrew-S-Rosen left a comment

Choose a reason for hiding this comment

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

Hi @tomdemeyere: Thank you for starting this PR! This already looks like it's on the way to a wonderful contribution.

From the recipe and settings side of things, everything looks good. I have no major comments there (just left a few minor comments).

My main question is what the purpose is for making the custom calculator as opposed to using (or inheriting from) ASE's Espresso calculator. Would you be able to walk me through your logic on this? I'm not necessarily opposed to a custom calculator, but it adds a fair bit of maintenance burden (and also won't benefit from any upstream PRs to the ASE Espresso calculator), so I want to make sure it is essential. I am a co-maintainer of ASE can help get things merged upstream if it's a matter of improving what already exists. I also left a few big-picture questions about the calculator, e.g. why not inherit from GenericFileIOCalculator per the ASE standard. I haven't given the calculator a thorough review yet in light of these questions.

Happy to answer any other questions you may have!

I think for the pp we shouldn't deal with env variables other than the QUACC ones since I am not aware of how different workflow managers deal with them. Covalent for example always uses env variables defined when the server starts and it does not seem intuitive to change them on the fly.

In principle, env vars shouldn't be a problem if you felt inclined. You would have the calculator (or, if a new calculator is not made, the base job function) call os.environ["myenvar"] = "yeah". You can see an example of that here for VASP, which seems to work okay. So long as the os.environ call is being made somewhere within the @job (@ct.electron), you should be good.

src/quacc/settings.py Outdated Show resolved Hide resolved
src/quacc/settings.py Outdated Show resolved Hide resolved
src/quacc/utils/dicts.py Outdated Show resolved Hide resolved
src/quacc/recipes/espresso/core.py Outdated Show resolved Hide resolved
src/quacc/recipes/espresso/core.py Outdated Show resolved Hide resolved
src/quacc/calculators/espresso/core.py Outdated Show resolved Hide resolved
src/quacc/calculators/espresso/keys.py Outdated Show resolved Hide resolved
Copy link
Member

@Andrew-S-Rosen Andrew-S-Rosen left a comment

Choose a reason for hiding this comment

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

Thanks!! I dropped a few comments here since I had some time to give it another look. Overall, it's looking good. Thank you for explaining the purpose of these executable "calculators" now as well --- that makes much more sense to me. Very neat that QE has that functionality.

src/quacc/recipes/espresso/core.py Outdated Show resolved Hide resolved
src/quacc/recipes/espresso/core.py Outdated Show resolved Hide resolved
src/quacc/settings.py Outdated Show resolved Hide resolved
src/quacc/recipes/espresso/neb.py Outdated Show resolved Hide resolved
src/quacc/calculators/espresso/calculators.py Outdated Show resolved Hide resolved
src/quacc/calculators/espresso/core.py Outdated Show resolved Hide resolved
src/quacc/calculators/espresso/calculators.py Outdated Show resolved Hide resolved
src/quacc/calculators/espresso/core.py Outdated Show resolved Hide resolved
src/quacc/calculators/espresso/keys.py Outdated Show resolved Hide resolved
src/quacc/calculators/espresso/core.py Outdated Show resolved Hide resolved
@Andrew-S-Rosen
Copy link
Member

Andrew-S-Rosen commented Dec 10, 2023

Here you go!

One small detail left: I still didn't change the quacc settings variables. I like the dictionary idea but at the same time I would like the possibility to change the path for each binary specifically in quacc.yaml

@tomdemeyere: Fantastic! Thank you for your very detailed revision. I really appreciate the time you put into this. There was one comment lost in the pile that I wanted to re-raise here. Namely, I find the name ph_job to be a bit confusing (since it relies on the name of an executable, which is not necessarily known by users new to Espresso). I assume this is a phonon job? If so, can we rename it phonon_job? Additionally, the additional_fields for that job is {"name": "ph.x static"}. Is this correct? I don't know that I'd call it a static calculation per se.

Finally, we'll need to move the very large strings out of tests/local/calculators/espresso/test_io.py into individual test files in that directory, which should then be read in. It is hard to update/understand the test_io.py module with such large blocks of text, and we should keep things small and modular. I am happy to do that, but I wasn't sure your answer to the above question.

Edit: I took care of all this!

@Andrew-S-Rosen
Copy link
Member

@tomdemeyere: Alright, this looks great to me! I am going to go ahead and have this merge once tests pass. Congratulations on this very nice PR, and thank you again for all of your insight. Please feel free to make further modifications as-needed in follow-up PRs.

@Andrew-S-Rosen Andrew-S-Rosen enabled auto-merge (squash) December 11, 2023 02:45
@Andrew-S-Rosen Andrew-S-Rosen merged commit 120829e into Quantum-Accelerators:main Dec 11, 2023
17 checks passed
@tomdemeyere
Copy link
Contributor Author

Thanks a lot for taking care of all of that! Looking at the commits it seems that I forgot quite a lot... I have a few branches that I might use to open follow-up PRs:

  • More espresso presets
  • First complex ph.x workflow (grid technique), using different executors
  • Espresso outdirs handling

In the meantime I also opened a merge request on the ASE master about our in house code ONETEP to prepare the integration with quacc.

@Andrew-S-Rosen
Copy link
Member

Thanks a lot for taking care of all of that! Looking at the commits it seems that I forgot quite a lot..

Not a problem! And the remaining stuff was mostly minor --- was happy to clean it up since you put in a lot of time and care into it as-is.

I have a few branches that I might use to open follow-up PRs:

Sounds good to me!

In the meantime I also opened a merge request on the ASE master about our in house code ONETEP to prepare the integration with quacc.

Awesome!! I had been wanting to add ONETEP recipes for a while. I have it compiled but never ran any test calculations myself. I'll be sure to review the ASE MR if Ask or someone else doesn't get to it in the next day or two.

@tomdemeyere tomdemeyere deleted the espresso branch November 15, 2024 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

First steps for espresso recipes
2 participants