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

NSW=1 --> NSW=0 in static, Cf in POTCARs #10

Merged
merged 9 commits into from
Nov 27, 2021
Merged

NSW=1 --> NSW=0 in static, Cf in POTCARs #10

merged 9 commits into from
Nov 27, 2021

Conversation

Andrew-S-Rosen
Copy link
Member

Quick Hello

Hi @utf! I'm opening this very minor PR in part to say hello and that I think atomate2 looks fantastic! I am really excited to start using it. Also a few small things.

Summary of Proposed Changes

  • I added Cf: Cf to the list of default pseudopotentials since it's available with the VASP PAW_PBE_54 potentials. It probably won't be relevant to anyone, but why not aim for completeness. You got all the other ones!
  • I changed NSW=1 to NSW=0 in your get_incar_updates() for StaticSetGenerator(). Was there a specific reason to use NSW=1 for the static calculation? One of my concerns is that other codes might look at NSW and think that it's doing a relaxation even though it's a static calculation.
    • Take the DriftErrorHandler() in Custodian, for example. It checks to see if EDIFFG >= 0 or NSW == 0. If this condition is met, it doesn't run the drift error checker; otherwise, it does. However, here, NSW=1 would make Custodian think it's a relaxation. Granted, Custodian is unlikely to actually do anything here, but it's more the premise that this might influence other codes in unexpected ways.

@utf
Copy link
Member

utf commented Nov 23, 2021

Hi @arosen93, great to have someone looking over atomate2. It is obviously still forming so any and all suggestions/PR are very welcome.

Thanks for catching the Cf pseudo potential. As for NSW = 1 this was a on purpose, as certain calculations require it (e.g., DFPT LEPSILON needs NSW = 1 and won't work with NSW = 0). That said, this does cause an incompatibility with custodian and that seems more difficult to fix than just setting NSW = 1 for certain calculation types. (I think that is still a bug in custodian and should probably be fixed).

Before I merge, can you add NSW = 1 to the custom lepsilon settings in StaticSetGenerator?

Important for DFPT that NSW = 1, not NSW = 0
@Andrew-S-Rosen
Copy link
Member Author

@utf, interesting! I didn't know that about DFPT. I see the thought process there, although my personal opinion is that it's better to set NSW=1 on an as-needed basis. Just changed the lepsilon settings.

@Andrew-S-Rosen
Copy link
Member Author

I figured that might break something along the way. Sorry for not running tests locally. I'll get the testing stuff set up tomorrow and can fix then. :)

@utf
Copy link
Member

utf commented Nov 23, 2021

Ah, the test data is now inconsistent. Should be as simple as editing the INCAR in the Si_bandstructure/static/inputs/INCAR file.

Also, to be consistent, we should probably also adjust NSW in the HSEStaticSetGenerator.

If I have a chance, I can take a look at this later.

@Andrew-S-Rosen
Copy link
Member Author

Andrew-S-Rosen commented Nov 24, 2021

Hi again @utf. That should do it now. Tests passed locally.

I had to change the INCARs in the following:

Si_hse_band_structure/hse_static
Si_band_structure/static

@utf
Copy link
Member

utf commented Nov 25, 2021

For the lint error. The easiest thing to do is to run pre-commit install when in the root atomate2 directory. Then, anytime you make a commit, any staged files will get linted automatically.

@Andrew-S-Rosen
Copy link
Member Author

Andrew-S-Rosen commented Nov 26, 2021

@utf: I guess I must have run the wrong pytest locally before. Clearly, there are some errors!

I wanted to run a few things by you here. Which INCARs should have NSW = 1? Is it just the Si_dielectric job? There are a bunch of test files with NSW = 1 in the INCARs right now, which is causing the pytest errors, e.g. the Si_elastic, Si_hse_optics, and Si_optics jobs. Is it okay if I change NSW to 0 in these INCARs? I'm looking back at Atomate1, and it seems like they have NSW = 0, but I've never run elastic or optics jobs before, so I didn't want to proceed without asking about this.

@utf utf merged commit bf6caff into materialsproject:main Nov 27, 2021
@Andrew-S-Rosen Andrew-S-Rosen deleted the rosen-atomate2 branch November 28, 2021 00:59
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.

None yet

2 participants