restart_mode should be set to from_scratch for a bands calculation.

[unkcpz]

After I update to 4.1.0 I get the warning "restart_mode should be set to from_scratch for a bands calculation.". I launch the band structure calculation from PwBandsWorkChain and I didn't override the inputs so I think it should be set. My question is where is this restart_mode set? I find from #475 that the restart_mode is removed there.

@mbercx is there any new changes in 4.1.0 that cause the issue? Let me know if more information needed.

[unkcpz]

Here is part of the logs output from running my workflow:

Report: [1958|PwBandsWorkChain|run_bands]: launching PwBaseWorkChain<1989> in bands mode                                                                                                                                                   
Report: [1956|PwBandsWorkChain|run_bands]: launching PwBaseWorkChain<1992> in bands mode                                                                                                                                                   
/home/jyu/miniconda3/envs/aiida-sssp-2.x/lib/python3.9/site-packages/aiida_quantumespresso/calculations/pw.py:171: 
UserWarning: `restart_mode` should be set to `from_scratch` for a `bands` calculation.                                    warnings.warn(f'`restart_mode` should be set to `from_scratch` for a `{calculation_type}` calculation.')
Report: [1989|PwBaseWorkChain|run_process]: launching PwCalculation<1995> iteration #1                                                                                                                                                     
/home/jyu/miniconda3/envs/aiida-sssp-2.x/lib/python3.9/site-packages/aiida_quantumespresso/calculations/pw.py:171: 
UserWarning: `restart_mode` should be set to `from_scratch` for a `bands` calculation.                                    warnings.warn(f'`restart_mode` should be set to `from_scratch` for a `{calculation_type}` calculation.')
Report: [1992|PwBaseWorkChain|run_process]: launching PwCalculation<1998> iteration #1     

[mbercx]

This is probably caused by the changes in #867, which was carelessly reviewed by yours truly. It's mainly because QE is a bit strange in its inputs. If you want to restart from the charge density you have to set restart_mode to from_scratch and startingpot to file. This always confuses me. ^^

For abands type calculation, the defaults of QE are perfectly fine, and the current implementation actually sets the restart_mode to restart, which we don't want. I guess we could build in all these cases, but we already have validation for this, so I'm inclined to just revert the changes in 0aba276. In general I'm inclined to be against setting parameters in work chains.

@sphuber can you explain why the changes in 0aba276 were necessary? How did you run into this?

[sphuber]

This resulted from a case from a user. They launched a calculation and it didn't finish. I pointed them to get_builder_restart as an easy way to get a builder that can be launched to restart and finish the calculation. However, as described in the commit message, this would still keep from_scratch and essentially the calculation would be redone and not finish. The commit detects the parent_folder in the inputs (which is set by get_builder_restart) and sets the restart_mode to be a restart. I think we should keep those changes but maybe add a clause to check for calculation == 'bands' in which case it is always set to from_scratch. Would that work? Ideally the logic is done maybe in get_builder_restart, but unfortunately that is defined on the CalcJobNode and we cannot customize it for PwCalculation nodes.

[unkcpz]

Is this UserWarning just a warning or does it basically changes the original workflow? Can still trust the new output (for my test workflow, the output is all the same, but I am concerned the workflow logic is changed)?
If I understand correctly, the solution is to add the "restart_mode" to input to suppress the warning, correct?

[unkcpz]

I think we should keep those changes but maybe add a clause to check for calculation == 'bands' in which case it is always set to from_scratch. Would that work?

I make a PR #891 for this, but I think this for sure change the original behavior of the workflow. The original set for bands calculation is using restart, which I think is correct. Change to from scratch would bring the issue to final result but not use the scf output which will make the calculation slow, correct?

I don't know where the restart is set for the bands originally, any clue on this? @sphuber @mbercx

[sphuber]

The original set for bands calculation is using restart,

This is actually incorrect for a non-scf calculation according to the QE docs.

[sphuber]

I think the logic should be correct now, doesn't it? The PwBaseWorkChain in the setup will automatically set restart if a parent_folder is specified unless the restart_mode has explicitly been defined in the inputs. So now the PwBandsWorkChain simply has to set restart_mode = from_scratch and the base workchain will not override. The PwCalculation will warn if restart_mode == 'restart' for a NSCF calculation, which is the right thing to do. We could think of making it an exception if we are convinced that this always be a bad idea. But maybe not the case.

[unkcpz]

Yes, I think the logic of calculation is correct. But I think for the workflow, the RestartType for band calculation should be from_scratch in first place, rather than override as I implemented. It seems it get set by detecting the parent_folder, if I understand correctly, the bands should not have parent_folder to restart.

[sphuber]

if I understand correctly, the bands should not have parent_folder to restart.

No, it definitely needs the parent_folder otherwise it is not an NSCF calculation.

The one more thing we could do is change PwBaseWorkChain.setup to always set restart_mode = from_scratch if the calculation is an NSCF type (essentially nscf and bands). This is the check that is being done in PwCalculation that emits a warning, but we could choose to enforce this in the base workchain.

[unkcpz]

Let's move discussion to the PR, I am wrong about this. The parent_folder is needed by band calculation since it is a nscf calculation. But the restart_mode should be from_scratch. My implementation is correct.

[mbercx]

This has been fixed in #902