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

Feat: Add run types for GW calculations #1105

Merged

Conversation

yanghan234
Copy link

Added GW-related configurations and modified utilities

This PR is a dependency of a PR in the atomate2 repo [WIP] feat: GW workflow with VASP. I previously submitted a PR in #1099 . However, I realized that I did not properly modified the calculation types, enums, etc. In this PR, these have been corrected.

Contributor Checklist

  • Added run types and calculations types for GW calculations
  • Modified the utils to identify GW calculations
  • I have run the tests locally and they passed.

Tagging @mkhorton for awareness.

@tsmathis
Copy link
Collaborator

Thanks for resubmitting @yanghan234, your last PR got submitted right as we were updating how we generate the calc_type/run_type enums, sorry for the extra work.

Re: the empty CONTCAR, when might that apply? Is that an edge case specific to the GW workflow?

@yanghan234
Copy link
Author

Thanks @tsmathis, yes, this is the edge case that I only oberve in the G0W0 calculation. In the G0W0 calculation, ionic steps are never updated. In addition, there is only one electronic step in such calculations, i.e. NELM = 1 or NELMGW=1. This is probably the reason that the CONTCAR is not written.

@@ -748,7 +748,10 @@ def from_vasp_files(
volumetric_files = [] if volumetric_files is None else volumetric_files
vasprun = Vasprun(vasprun_file, **vasprun_kwargs)
outcar = Outcar(outcar_file)
contcar = Poscar.from_file(contcar_file)
try:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given your comment about when CONTCAR is not written, could you change this to:

if not os.path.isfile(contcar_file) and vasprun.parameters.get("NELM",60) == 1:
    contcar = Poscar(vasprun.final_structure)
else:
    contcar = Poscar.from_file(contcar_file)

This should help separate cases where something has gone wrong (CONTCAR missing for any reason) vs. expected behavior when NELM is 1

@@ -140,4 +140,5 @@ TASK_TYPES:
- Deformation
- Optic
- Molecular Dynamics
- GW Band Structure
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I would put GW Band Structure as a task type. I think you want to add GW to the run types based on the ALGO. Then the corresponding task type would be NSCF Line or NSCF Uniform, which is what we use to indicate band structure calculations along a line in the Brillouin zone (NSCF Line), or using a regular k-point grid (NSCF Uniform). 'Static' is also an acceptable fallback

Also there need to be features in emmet.core.vasp.calc_types.utils to parse these, they were in your original PR, can you migrate these over?

@yanghan234
Copy link
Author

Thanks @esoteric-ephemera, please give me some time to fix them. Meanwhile, I realize that testing/check-enums check fails because it cannot find the branch in the forked repo under my personal account. Is this a problem with the CI setup, should I be concerned about this?

@tsmathis
Copy link
Collaborator

tsmathis commented Sep 26, 2024 via email

@yanghan234
Copy link
Author

@esoteric-ephemera Thanks for the comments. I have updated the codes according to your comments. They are not exactly the same as what you proposed, so please take a look.

@@ -748,7 +749,13 @@ def from_vasp_files(
volumetric_files = [] if volumetric_files is None else volumetric_files
vasprun = Vasprun(vasprun_file, **vasprun_kwargs)
outcar = Outcar(outcar_file)
contcar = Poscar.from_file(contcar_file)
if (
os.path.getsize(contcar_file) == 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we change this to not os.path.isfile(contcar_file) or os.path.getsize(contcar_file) == 0? Or is the CONTCAR written for GW but it's just empty?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, the case is the CONTCAR written for GW but it's just empty

emmet-core/emmet/core/vasp/calc_types/utils.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Oct 11, 2024

Codecov Report

Attention: Patch coverage is 91.42857% with 3 lines in your changes missing coverage. Please review.

Project coverage is 89.97%. Comparing base (2a12298) to head (6fe3a88).

Files with missing lines Patch % Lines
emmet-core/emmet/core/vasp/calc_types/utils.py 33.33% 2 Missing ⚠️
emmet-core/emmet/core/vasp/calculation.py 75.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1105   +/-   ##
=======================================
  Coverage   89.97%   89.97%           
=======================================
  Files         143      143           
  Lines       13723    13756   +33     
=======================================
+ Hits        12347    12377   +30     
- Misses       1376     1379    +3     

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

@esoteric-ephemera
Copy link
Collaborator

@tsmathis while we're figuring out a secure solution for pushing to external forks, is it OK if I merge this? CI on main should fix the enum formatting as needed

@tsmathis
Copy link
Collaborator

Yep all good on my end

@esoteric-ephemera esoteric-ephemera merged commit 42606d4 into materialsproject:main Oct 11, 2024
7 of 8 checks passed
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