add should_run_scf for PwBandsWorkChain

[superstar54]

This PR sets the scf step as an option in the PwBandsWorkChain.

Use case:

• In aiidalab-qe, we want to run one scf in the main workflow, then pass the remote_folder to plugins, such as bands and pdos. This requires that the scf calculation is an option in the PwBandsWorkChain.
  #Run one SCF calculation for all plugins aiidalab/aiidalab-qe#631

[sphuber]

Thanks @superstar54 . Does this currently actually work? I believe if you skip the SCF step, then there won't be a parent_folder as input to the bands step, and the user also cannot specify it in the inputs since it is excluded. Besides that, as @mbercx mentioned during the meeting, if we strip out the SCF step, it is not much of a workflow anymore. If the user already has an SCF completed and wants the band structure, wouldn't they just be better off running a PwBaseWorkChain specifying the input parameters themselves? Using the get_builder_from_protocol they can get all inputs preset and then just switch the calculation to bands. The logic in PwBandsWorkChain specific to the bands is not very intricate anyway, it is just to define the total number of bands, which is just based on a top-level user input anyway.

[sphuber]

How would the parent_folder be defined though? It is excluded from the bands input namespace, and if scf is skipped this will therefore not be set?

[superstar54]

Thanks for pointing out this. I didn't notice that the parent_folder is excluded. I run a simple example with Si by setting the parent_folder, it's strange that no error is reported, and the process finished successfully! Here is the process detail:

$ verdi process show 28038
Property     Value
-----------  ------------------------------------
type         PwBandsWorkChain
state        Finished [0]
pk           28038
uuid         94b2d315-6a72-4f73-ae87-5a3b50d86d07
label
description
ctime        2024-03-21 15:34:17.077194+00:00
mtime        2024-03-21 15:34:24.276367+00:00

Inputs                  PK     Type
----------------------  -----  -------------
bands
    pw
        code            216    InstalledCode
        pseudos
            Si          19     UpfData
        parameters      28034  Dict
        parent_folder   28029  RemoteData
    max_iterations      28035  Int
bands_kpoints_distance  28036  Float
clean_workdir           28037  Bool
structure               28020  StructureData

Outputs                 PK  Type
-------------------  -----  -------------
band_parameters      28054  Dict
band_structure       28052  BandsData
primitive_structure  28042  StructureData
seekpath_parameters  28040  Dict

Called       PK  Type
--------  -----  ---------------------------
seekpath  28039  seekpath_structure_analysis
bands     28046  PwBaseWorkChain

Log messages
---------------------------------------------
There are 3 log messages for this calculation
Run 'verdi process report 28038' to see them

You see that the parent_folder is set. Here is the node graph

[mbercx]

@superstar54 do you still think this is a value feature to have, considering @sphuber's comments above? Happy to discuss your use case, but if not I'll close this PR if that's alright.

[superstar54]

Hi @mbercx , thanks for looking at this PR again. Initially, we wanted the aiida-qe to provide such an option, so that we could use the workchain from aiida-quantumespresso directly. But recently, we wrote a lot of workchains inside the aiidalab-qe directly, either wrapping the one from aiida-qe, or creating a new workchain. So it's fine for me to close this PR, and we can write this one inside aiidalab-qe.

Apart from aiidalab-qe, this could also be a possible use case: if a user runs a Pdos calculation using PdosWorkChain, later he wants to run a band calculation using the PwBandsWorkChain, he needs to run the scf calculation again, even though the pdos already ran it. Of course, as suggested by @sphuber , the user can run a PwBaseWorkChain by manually setting the number of bands. But if the PwBandsWorkChain can skip the scf and accept the parent_folder from other calculation, this will be easy for the user. If you think feature is not needed, then please close this PR.

[mbercx]

@superstar54 I'm going to add this to the v5.0 milestone. I'm also considering removing the PwRelaxWorkChain from the PwBandsWorkChain. Idea being that I prefer to have more atomic work chains with a clear purpose, without too much overlap between them. These are then building blocks that can be tied together, e.g. using the WorkGraph.

While reworking the PwBandsWorkChain, I'll also add some documentation etc. Hopefully that will give me enough clarity to decide if the changes you made here provide a legitimate use case we want to integrate. :)

[superstar54]

Hi @mbercx , thanks for looking at this again.

[mbercx]

@superstar54 I've had some time to think about this, and I'm now leaning towards accepting these changes (EDIT: or, I was; see edit below 😅 ), especially if we implement #1146 (for which I'd love to hear your opinion btw ^^).

Although it's true you only run a single bands calculation in case you provide the SCF parent folder, the workflow does three other things to make your life easier:

1. Create a k-path with seekpath.
2. Set the number of bands based on the SCF.
3. Provide protocols for running the bands calculation.

What I would do is make the API more explicit and add a top-level scf_folder input, as I propose for the PdosWorkChain here:

#1139

EDIT: Looking at the implementation, I do wonder about the logic for seekpath. Now seekpath is run when the bands_kpoints is not provided, and does two things:

1. Update the structure: This can both primitivize and normalise (make the atomic positions more exact) the structure.
2. Provide an explicit k-points path to run.

This is of course why seekpath is run before the SCF. So you calculate the charge density for the correct structure that you have the k-path for. This means that advantage [1] above is not valid: the user will have to provide bands_kpoints in case they want to run the bands calculation on top of an SCF.

This also means the use case you describe is not so simple. the PdosWorkChain does not run seekpath, so in case the user wants to have benefit [1], they need to rerun the SCF for the results to be correct.

So, I'm not quite convinced I no longer want to make the SCF optional (sorry for the rollercoaster ride!). The benefits it brings to the user are not worth the added complexity on the API and code, and potentially can lead to an incorrect calculation if the user doesn't understand what they are doing. So I'll close this PR, but we can always reopen it if you disagree. :)

More importantly: are we sure we are doing this correctly in the QEapp? I suppose you'd run seekpath right after the relaxation, and then run the pdos and bands?

[superstar54]

Hi @mbercx, I see your point that if the user provides the parent_folder for SCF while also requiring to run' seekpath', there could be a mismatch in the structure. However, this can be avoided by validating the inputs.

My understanding is: this is an opt-in feature. The users still run the original workflow, but if they want to avoid duplicate scf (as in aiidalab-qe case), they can choose this feature, while the validation of the inputs also makes sure they are not running seekpath, and require providing the bands_kpoints at the same time.

I can add this validation if you think this is reasonable.