aiidalab / aiidalab-qe

AiiDAlab App for Quantum ESPRESSO
https://aiidalab-qe.readthedocs.io/
MIT License
12 stars 14 forks source link

Change `nbnd` logic for `QeAppWorkchain` #362

Open mbercx opened 1 year ago

mbercx commented 1 year ago

Is your feature request related to a problem? Please describe.

See https://github.com/aiidateam/aiida-quantumespresso/issues/887

The main issue is the fact that nbnd is set to the "current" number of bands for the NSCF step of the PDOS work chain:

https://github.com/aiidalab/aiidalab-qe/blob/2e4c7faa50240071e80d855969a49e0643154412/src/aiidalab_qe_workchain/__init__.py#L303-L306

The reason for doing this is related to the fact that for metals, the default number of bands chosen by QE is sometimes not enough, which is handled by a check in the PwBaseWorkChain. So the idea was to grab the number of bands from the final calculation in the relaxation:

https://github.com/aiidalab/aiidalab-qe/blob/2e4c7faa50240071e80d855969a49e0643154412/src/aiidalab_qe_workchain/__init__.py#L236-L241

and store it in the context to be set later.

However, when running an insulator, current_number_of_bands will be the number of occupied states. In some cases, this results in outright failures when running the NSCF, but in general we probably don't want to limit the number of bands for the NSCF. We probably just want to either not set the number of bands so QE takes a reasonable default, or set the number of bands to the same one used for the band structure calculation. Now when calculating both the band structure and PDOS the latter won't have the higher lying unoccupied states, e.g.:

Screenshot 2023-03-01 at 18 21 42

Describe the solution you'd like

For metals: it would still be somewhat beneficial to store the number of bands used for the relaxation in the context _if this has been increased due to the PwBaseWorkChain.sanity_check_insufficient_bands process handler_. In this case it can be used for the SCF calculation, and potentially for the NSCF calculation as well.

For insulators: If the material is truly an insulator, QE can just run the relaxation with the number of occupied states and occupations = fixed, and the PwBaseWorkChain.sanity_check_insufficient_bands process handler should never be called. Hence storing the number of bands for later use is not necessary here.

In all cases: We may want to consider running the PDOS with the same number of bands as the band structure calculation if both are calculated. However, the current default number of bands from the protocol (SCF x 3) is probably rather too high for most purposes. Maybe we can reduced this to a more reasonable number (e.g. 1.5).

unkcpz commented 1 year ago

Thanks a lot for the detail, very clear to me which options we have to make it work. I'll give it a try in the following sprint.