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 setting VASP_RUN_DDEC6: bool = False #587

Merged
merged 10 commits into from
Nov 8, 2023
Merged

Add setting VASP_RUN_DDEC6: bool = False #587

merged 10 commits into from
Nov 8, 2023

Conversation

janosh
Copy link
Member

@janosh janosh commented Oct 20, 2023

Similar to existing setting VASP_RUN_BADER, allows saving DDEC6 charge analysis automatically in any VASP TaskDoc.

VASP_RUN_BADER: bool = Field(

Related PR materialsproject/emmet#869

@janosh janosh added feature A new feature being added vasp Vienna Ab initio Simulation Package labels Oct 20, 2023
@@ -84,6 +84,11 @@ class Atomate2Settings(BaseSettings):
description="Whether to run the Bader program when parsing VASP calculations."
"Requires the bader executable to be on the path.",
)
VASP_RUN_DDEC6: bool = Field(
Copy link
Member

Choose a reason for hiding this comment

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

It also requires an environment variable DDEC6_ATOMIC_DENSITIES_DIR. I mean, by default it assumes local directory but it's unlikely the user will copy the atomic densities to the local dir if not directed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we were discussing over at materialsproject/emmet#869 (comment). I think it would be best not to bother the user with this and just handle it automatically.

Copy link
Member

Choose a reason for hiding this comment

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

I see. Just double-check the licensing that this would be permitted, assuming you mean that you'll ship it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not ship in the pymatgen package since it would burden many non-users of chargemol to download those files. But once they're in the repo, we have a stable raw.github.com URL from where we could wget them.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that's smart. But one thing to note is that not all compute nodes have access to the network, so you might want to have a fallback option.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, fallback option for no-network situations is on the TODO list.

Copy link
Member

Choose a reason for hiding this comment

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

Oh yes, please. Most computing centers in Europe don't have any outside connection on the compute nodes.

Copy link
Member

Choose a reason for hiding this comment

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

Or at Princeton... 😉

@codecov
Copy link

codecov bot commented Oct 20, 2023

Codecov Report

Merging #587 (8705e24) into main (3764841) will decrease coverage by 0.18%.
Report is 41 commits behind head on main.
The diff coverage is 51.85%.

❗ Current head 8705e24 differs from pull request most recent head 06788d0. Consider uploading reports for the commit 06788d0 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #587      +/-   ##
==========================================
- Coverage   75.57%   75.40%   -0.18%     
==========================================
  Files          83       83              
  Lines        6793     6805      +12     
  Branches     1001     1008       +7     
==========================================
- Hits         5134     5131       -3     
- Misses       1348     1360      +12     
- Partials      311      314       +3     
Files Coverage Δ
src/atomate2/cp2k/schemas/calculation.py 70.46% <ø> (ø)
src/atomate2/forcefields/schemas.py 92.00% <100.00%> (ø)
src/atomate2/settings.py 100.00% <100.00%> (ø)
src/atomate2/vasp/jobs/base.py 78.87% <18.75%> (-17.62%) ⬇️

... and 11 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new feature being added vasp Vienna Ab initio Simulation Package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants