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

Espresso: streamline file copying / additional recipes #1974

Merged

Conversation

tomdemeyere
Copy link
Contributor

@tomdemeyere tomdemeyere commented Apr 3, 2024

Summary of Changes

This pull request introduces several improvements and new features to Quacc Espresso, enhancing the user experience and expanding its capabilities:

Streamlined Input/Output Naming:

  • Quacc Espresso now enforces predefined names for all input and output files removing the little freedom users had for this. This ensures consistency across different calculations and reduce the potential for errors.

Intelligent File Copying:

  • When users provide a prev_dir argument, Quacc will now determines which files to copy based on the binary and input data. This will reduce the amount of data being copied overall, I think this was one of your suggestions on a previous merge request.

New Recipes for Electron-Phonon Calculations:

  • Two new recipes have been added to perform electron-phonon calculations and Fourier interpolation of the phonon potential. It's a little step toward future compatibility with codes such as EPW.

Some tests have been modified to be more robust, some internal functions have been modified as well.

note to self: the two recipes have to be added to the list of recipes

Checklist

  • I have read the "Guidelines" section of the contributing guide. Don't lie! 😉
  • My PR is on a custom branch and is not named main.
  • I have added relevant, comprehensive unit tests.

Notes

  • Your PR will likely not be merged without proper and thorough tests.
  • If you are an external contributor, you will see a comment from @buildbot-princeton. This is solely for the maintainers.
  • When your code is ready for review, ping one of the active maintainers.

Copy link

codecov bot commented Apr 6, 2024

Codecov Report

Attention: Patch coverage is 98.19820% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 99.21%. Comparing base (bd23ed3) to head (929d2ce).

Files Patch % Lines
src/quacc/calculators/espresso/utils.py 96.72% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1974      +/-   ##
==========================================
- Coverage   99.29%   99.21%   -0.08%     
==========================================
  Files          81       81              
  Lines        3268     3325      +57     
==========================================
+ Hits         3245     3299      +54     
- Misses         23       26       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tomdemeyere
Copy link
Contributor Author

I didn't generalize the list[SourceDirectory] behaviour, for now this is espresso only

I added some general purpose espresso documentation but I don't it is in the right place, (it is in quacc.calculators.espresso.espresso.Espresso for now)

Other than that, this should be ok to go if you are happy with it

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.

Thank you very much for your contribution, @tomdemeyere! This is all looks great to me. I have left some minor comments below, all of which are trivial and should not take more than a few minutes. After they are addressed, I'll be happy to merge!

I didn't generalize the list[SourceDirectory] behaviour, for now this is espresso only

Sounds good.

I added some general purpose espresso documentation but I don't it is in the right place, (it is in quacc.calculators.espresso.espresso.Espresso for now)

Agreed, as commented below, that's probably not the right place. We should either clarify the recipe docstrings themselves (if needed) or, if it's meant to be more of a tutorial/guide, it should be a dedicated section in the quacc documentation. We don't currently have tutorials for the different codes (#1518), but there's nothing stopping us from adding a mini Espresso section if we feel like it. That would probably be a bit more detailed than the text provided in this PR though. I would say let's remove it for now.

Other than that, this should be ok to go if you are happy with it

👍

src/quacc/calculators/espresso/espresso.py Outdated Show resolved Hide resolved
src/quacc/recipes/espresso/_base.py Outdated Show resolved Hide resolved
src/quacc/recipes/espresso/_base.py Outdated Show resolved Hide resolved
src/quacc/recipes/espresso/_base.py Outdated Show resolved Hide resolved
src/quacc/recipes/espresso/_base.py Outdated Show resolved Hide resolved
src/quacc/recipes/espresso/bands.py Outdated Show resolved Hide resolved
src/quacc/recipes/espresso/bands.py Outdated Show resolved Hide resolved
src/quacc/recipes/espresso/phonons.py Outdated Show resolved Hide resolved
src/quacc/recipes/espresso/phonons.py Outdated Show resolved Hide resolved
src/quacc/recipes/espresso/phonons.py Outdated Show resolved Hide resolved
@Andrew-S-Rosen
Copy link
Member

Also, if you can fix the merge conflict that'd be great, but I'm happy to do so if that's too bothersome.

@Andrew-S-Rosen
Copy link
Member

Good to go? (The Deepsource X will resolve once this is merged)

@tomdemeyere
Copy link
Contributor Author

Yes good to go

@Andrew-S-Rosen
Copy link
Member

Beautiful. Thank you very much!

@Andrew-S-Rosen Andrew-S-Rosen merged commit 06faf4e into Quantum-Accelerators:main Apr 9, 2024
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants