Vasp calculator update

[Felixrccs]

• Enabled per configuration kpoint setting (important for slabs, because VASPs kspacing does not work)
• Added converged as retrievable propertie (If VASP reaches the maximum number of electronics steps, it returns the energies and forces even if not converged, and without any error.)

[bernstei]

Enabled per configuration kpoint setting (important for slabs

Please take a look at #254 and figure out whether what you want can be done through that general mechanism.

because VASPs kspacing does not work)

In what sense?

[Felixrccs]

Probably my description was not really clear. kspacing does work, but does not make sense for slabs, since kspacing is allways equivalent in xyz, for slabs one usually wants to set the kpoints to 1 in one of those directions.

254 does exectly what I want, thank for that change, I look forward to see it in the main branch. I tested the brach as well and did some small changes #256.

I think adding the converged information is still a benefit. I dont want to go trough my OUTCAR files again to check if everything is converged or not.

[bernstei]

Probably my description was not really clear. kspacing does work, but does not make sense for slabs, since kspacing is allways equivalent in xyz, for slabs one usually wants to set the kpoints to 1 in one of those directions.

OK, I understand - note that often the normal direction is much larger than the others, so it will end up with 1 k-point, but I agree that you can't count on that.

I've been considering writing a more generic (i.e. independent of the specific calculator) k-point mesh generator, which could then do stuff like recognize that you turned PBC off for the normal direction and always go 1 k-point in that direction. We could have all our DFT calculator wrappers call it. The VASP calculator, e.g. already magically switches pbc to True because otherwise the ASE calculator fails, and switches back when it's done. It would violate our general tendency to not re-implement functionality that's already basically in the individual DFT calculators. Does anyone think it'd be worth it?

254 does exectly what I want, thank for that change, I look forward to see it in the main branch. I tested the brach as well and did some small changes #256.

Great. I'm just waiting on someone to have time to write a test for #254.

I think adding the converged information is still a benefit. I dont want to go trough my OUTCAR files again to check if everything is converged or not.

This I definitely agree with, but you should consider (if the ASE governance process is currently functional - @jameskermode ?) actually adding that to the underlying ASE calculator, rather than to the workflow wrapper.

[Felixrccs]

've been considering writing a more generic (i.e. independent of the specific calculator) k-point mesh generator, which could then do stuff like recognize that you turned PBC off for the normal direction and always go 1 k-point in that direction.

I think this is a cool wfl feature to include. In our group everyone is using their own implementation to keep kpoint density consistent. This would than be redundant.

[bernstei]

Turns out that this is a bit more subtle than I first expected, because it means that it's always a per-configuration calculator (since you have to pass an explicit k-point list, and that's in the calculator parameters). I'm thinking about whether it can be done cleanly within wfl.

[bernstei]

Do we think that just providing a mesh size is sufficient? I.e. do all DFT codes accept a set of 3 integers (plus maybe a gamma vs. Monkhorst-Pack flag) and create their own mesh?

[bernstei]

I've got some tentative code implementing this, and using it in the wfl Vasp wrapper. While doing that I modified handle_nonperiodic, and then I went to look at what other DFT calculator wrappers did with respect to this and other k-point related issues, and it's all over the place. We may need to get together to discuss rationalizing all of it.

[Felixrccs]

Do we think that just providing a mesh size is sufficient? I.e. do all DFT codes accept a set of 3 integers (plus maybe a gamma vs. Monkhorst-Pack flag) and create their own mesh?

I am familiar with QE and VASP, there this would be sufficient; However to implement this throughout all DFT calculators could be complex and prawn to errors (Also regarding different versions).

[bernstei]

We'll have to make whoever wrote each wrapper deal with this for that code, which presumably they know. And, in real life, things as basic as k-point mesh handling very rarely change with version, in my experience.

[bernstei]

Besides - if a wrapper doesn't want to support this new universal kspacing, that's fine. It shouldn't lose anything it already had before, just won't gain this compatibility.

[bernstei]

@Felixrccs If you make this PR be the converged result only, I'm happy to merge it, and we'll deal with the kpoints via the other mechanism(s), i.e. per-config calculator args and universal k-point spacing definitions.

[Felixrccs]

Ok I removed the kpoint changes.

However the 'converged' property will only work for VASP (its calulator dependend) but its also only needed for VASP. (all the other packages give a calculation error if not converged.) So you have to decide weather the calculators/utils.py is the right place to keep this change or if there might be a smarter way.

[bernstei]

I think the best place for it is in the ASE Vasp calculator, possibly with a flag that can also make it raise an error if that's what's commonly done in the other calculators. But I'm not at all sure it'll take a reasonable amount of time to be added to the main ASE repo. What do you think about merging this one into wfl, and also doing a PR for main ASE Vasp that will make it raise an error for failed convergence, more like the other DFT calculators (possibly controlled with a constructor parameter, and maybe add such a parameter to other calculators as well)?

For wfl I'm fine with keeping what it's doing now, if that's what you think is most useful for you. There's always a tension between improving consistency (e.g. making the wfl Vasp wrapper raise an error, like you say other calculators do), or keeping things simple, and not having the wrapper alter the behavior of the underlying calculator any more than necessary.

[bernstei]

@Felixrccs are you happy to merge this one as is, or do you want to make more changes to it before we merge?

[Felixrccs]

Let’s merge it as it is. I‘ll probably write an issue on ASE for this. If ase does something we can change it to something better at some point.