SiestaCalculation bandskpoints input does not accept KpointsData with cartesian=False

[pfebrer]

I don't know why, in this line of the parser it is enforced that bandskpoints must have a cell and cartesian coordinates can be extracted:

https://github.com/albgar/aiida_siesta_plugin/blob/2d2104901f3a5ca6403c47df84c80c8e785ae68e/aiida_siesta/parsers/siesta.py#L388

I want to use a single KpointsData node as the path for different calculations, regardless of the cell. Therefore the points are specified in units of the reciprocal lattice vectors.

The calculation runs fine, but then when the output is parsed it exits with an exception, making it not even possible to access the other outputs of the calculation.

[pfebrer]

I think it was just a workaround for https://github.com/aiidateam/aiida-core/pull/2779, which is already solved.

This goes all the way back to this commit from @albgar, so maybe he can give a hint on why this is needed/ was introduced?

[bosonie]

I can answer. The imposition to have cartesian=True is due to the fact that this is needed to plot the bands with the correct distance between points. In the documentation it is specified that the bandskpoints needs a cell. Therefore I would say that the bug is that I don't check internally that bandskpoints has a cell and that I do not perform all the steps of the bands set up under the try in the parser. Is it a problem to pass the structure to the bandskpoints in your use case?? I never encountered such a problem

[pfebrer]

Can't you take the cell from the input structure when you are parsing the output? It would be more convenient in my opinion, because for the actual calculation of the bands it makes sense to just pass a path in units of the reciprocal vectors.

Then you can reuse the same path for different structures (e.g. same structure with different scales).

[pfebrer]

Actually, you need to take it from the input/output structure because otherwise when you relax the structure there's no way to set a cell beforehand, is it?

[bosonie]

This is a good point I never though about. We have to reconsider the entire bands part. Especially the use of tools in order to automatically get the kpoints path connecting high symmetry points. In fact seekpath imposes the use of some conventions in the cell

[bosonie]

I'm reflecting a bit more and the conclusions are: 1) The current implementation it is ok as far as we don't have relaxation with variable cell. 2) If there is a variation of the cell, the parsing is wrong because it does not parse what siesta creates (siesta uses the final structure in order to define the kpoints distances, here we don't). 3) But in general...does it make sense to allow to specify kpoints for bands when there is a variation of the cell?? I don't think so, because you do not know "a-priori" where the cell goes! However siesta (the code) allows it and you do it at your own risk. 4) The SiestaCalculation can not be smarter than siesta code, however workchains can! Therefore I think the real bug is that we should have set since the beginning that SiestaBaseWorkChain, if bands are requested, runs separately the bands calculations after the relaxation is done. This was proposed by @albgar but I never seen the benefits till now. This might also imply a different way to chose the bands inputs of the workchain.

[bosonie]

The actions I would take are: 1) Report on the release notes of current release that there is a bug and bands calculations and relaxation with variable cell are not compatible in SiestaCalculation, SiestaBaseWorkChain, BandGapWorkChain and "STMWorkChain". 2) Make a new patch release with the following changes:

a) Introduce an error in `SiestaCalculation` saying that bands calculations and relaxation with variable cell are not possible.  Workchains will cover this cases.
b) Adapt the `SiestaBaseWorkChain` (or at least the `BandGapWorkChain`) in order to run separately the bands calculation and allow to pass to `bandskpoints` the possibility of `automatic` or something like that.

Details to be defined, but I would appreciate your comments on this general idea.

[pfebrer]

I don't think it is that stupid to let people relax their structure and specify some bandskpoints. Of course if a phase transition happens the bands path for your initial structure will be wrong, but most of the times (at least in my experience) you relax a structure expecting just a rescaling of your lattice vectors.

Edit: Maybe you could differentiate between relaxations and molecular dynamics, for example?

allow to pass to bandskpoints the possibility of automatic or something like that

I think this might be useful in cases where you are not sure what will happen.

In general, after reading your points I still think that allowing bandskpoints without a cell (which is automatically incorporated using the input/output structure) can be convenient, because:

1. Then you can have a single node stored in your database and it is better organized. E.g.: I stored my Y -> Gamma -> X path as "NPG bands path", and anytime I want to calculate bands for this material (or a variant of it) I would just need to load_node("NPG bands path").
2. I feel like you are heading towards a "highly recommended" use of seekpath (which could be automatized). However, this would only work if seekpath was perfect. In my case, it doesn't work because it can't define paths for 2D materials. Also when you introduce a defect in your system, I guess the simmetry group changes and therefore seekpath proposes a different path, or not?

All in all, I'd be happy if you let me pass my own non-structure-bound paths even if I have to use an input like DANGEROUS_bandskpoints or something like that :)

[bosonie]

Ok. Can you tell me how you specify the kpoints? Do you use the legacy tool? Do you create them just as equidistant points? You need to pass them all, not only the high symmetry points.

[pfebrer]

I didn't find a good way to do it in aiida. Legacy sounded not so good, so I created the points using sisl's BandStructure, but I could have done it simply using numpy's linspace. Although it seems strange that KpointsData doesn't have some definition of path to save memory in the database.

[bosonie]

I know...we are discussing it here aiidateam/aiida-core#4391 Legacy is the way to go at the moment, as explained in our documentation https://aiida-siesta-plugin.readthedocs.io/en/latest/plugins/siesta.html#inputs under bandskpoints. Anyway, I'll fix this over the weekend.