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 VASP setting for the dftd4 vdW functional and extend PBE_64 support. #3967

Merged
merged 7 commits into from
Aug 24, 2024

Conversation

hongyi-zhao
Copy link
Contributor

@hongyi-zhao hongyi-zhao commented Aug 2, 2024

This is an enhancement and re-implementation of #3899 and the PRs suggested to include in it.

Add VASP setting for the dftd4 vdW functional and extend PBE_64 support:

  • Add IVDW: 13 for dftd4 in vdW_parameters.yaml
  • Support PBE_64 functional in MPScanRelaxSet, MVLRelax52Set, MVLScanRelaxSet, and LobsterSet
  • Update documentation and error messages to include PBE_64
  • Add warning for LobsterSet when using PBE_52 and PBE_64 POTCARs with PBE_54 basis functions
  • Update relevant test cases to accommodate PBE_64 support

- Add IVDW: 13 for dftd4 in vdW_parameters.yaml
- Support PBE_64 functional in MPScanRelaxSet, MVLRelax52Set, MVLScanRelaxSet, and LobsterSet
- Update documentation and error messages to include PBE_64
- Add warning for LobsterSet when using PBE_64 POTCARs with PBE_54 basis functions
- Update relevant test cases to accommodate PBE_64 support
@hongyi-zhao
Copy link
Contributor Author

hongyi-zhao commented Aug 2, 2024

I've installed the dev and optional dependencies per the guidance here. There were no problems with my local pre-commit and pytest checks. So, I don't know why the PR failed to pass some of the tests.


def __post_init__(self) -> None:
super().__post_init__()
warnings.warn("Make sure that all parameters are okay! This is a brand new implementation.")

if self.user_potcar_functional == "PBE_64":
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if self.user_potcar_functional == "PBE_64":
if self.user_potcar_functional == "PBE_64":

Could you add PBE_52 here as well? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thank you for reminding me.

Copy link
Member

Choose a reason for hiding this comment

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

could you resolve the merge conflicts? sets.py is now split into submodules so it's a matter of re-applying your changes to the right file

Copy link
Contributor Author

@hongyi-zhao hongyi-zhao Aug 9, 2024

Choose a reason for hiding this comment

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

Currently, this has been reverted by #3989.

@JaGeo
Copy link
Member

JaGeo commented Aug 2, 2024

And, @hongyi-zhao , thank you!

Copy link
Member

@JaGeo JaGeo left a comment

Choose a reason for hiding this comment

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

From my side, the changes are fine

Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

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

looks good! thanks @hongyi-zhao 👍

@janosh janosh force-pushed the master branch 2 times, most recently from e3fbc67 to 41e6d99 Compare August 3, 2024 19:01
@shyuep
Copy link
Member

shyuep commented Aug 9, 2024

There seems to be some conflicts that need to be resolved first.

@hongyi-zhao
Copy link
Contributor Author

hongyi-zhao commented Aug 9, 2024

@shyuep Done.

@hongyi-zhao hongyi-zhao force-pushed the add-pbe64-dftd4-support branch from dcddc12 to 0a3359b Compare August 9, 2024 23:34
@janosh janosh added enhancement A new feature or improvement to an existing one io Input/output functionality vasp Vienna Ab initio Simulation Package labels Aug 23, 2024
@janosh janosh enabled auto-merge (squash) August 23, 2024 09:45
@janosh janosh merged commit 6f280a9 into materialsproject:master Aug 24, 2024
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature or improvement to an existing one io Input/output functionality vasp Vienna Ab initio Simulation Package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants