Feature/alternative bands wc #242
Conversation
On the lines of the common structure relaxation workflow, we implement the abstract classes for the creation of the bands workflow with common interface (inputs and outputs). Each code will need to make its own implementation. This commit includes the siesta implementation.
|
Some notes that must be remembered:
|
I think we should simply remove |
The original implementation of the `InputGenerator` abstract class was setting this class as child of `ProtocolRegistry`. This is not ideal since there might be input generators that do not require protocols. The dependence is removed and the `RelaxInputGenerator` has now both `ProtocolRegistry` and `InputGenerator` as parents.
No need for constructor in `CommonRelaxInputGenerator` if it doesn't add any functionality. Base class constructors will be called automatically. Also make `process_class` an explicit keyword argument for `InputGenerator`. Finally, reverse the order of subclasses `CommonRelaxInputGenerator` such that the `process_class` argument is only passed to the generator class and not the `ProtocolRegistry`.
|
by using the |
|
One potential problem with this approach. I thought the idea for this setup was that it would allow the SCF (with relax) to be performed with one code and the bands computed with another. However, that would mean that the bands workchain generator can recreate the inputs from a remote folder that is created by an arbitrary plugin, which I don't think is possible. I think it is only feasible for a plugin to determine the inputs from a remote folder created by its own common relax workflow. So I guess the approach presented here either limits cross-code use or we drop this design (even though taken by itself I prefer this as well compared to the other). |
|
Thanks @sphuber for the comment. I think we can overcome the issue you mention inside the code-agnostic I actually believe that this is the best solution, even better than the other bands implementation (#235), because it guarantees consistency among the used inputs of the input generator among the two steps. In the other bands implementation it was up to the user to select again the same protocol etc. that was used for the relaxation also for bands. |
|
Hi @sphuber, regarding your comment
I don't think this was the original reason for this design. The reason was to avoid to repeat again all inputs for the relaxation also for the bands, meaning the implementation needs to validate in some way that it's all compatible. As a note, I don't think running bands with a different code than SCF is really an interesting usecase. |
Indeed, CASTEP typically runs band in single calculation without restart, but running as two separate calculations is also fine (e.g. reusing the density). I also know that for VASP doing hybrid fucntional band structure is also a single calculation. But even if a code doesn't need the first SCF calculation, the worst case would just be performing an extra calculation, and very often the remote folder can be reused to speed up the second one. So I think this design is fine here. |
This is what we agreed on in the first meeting that discussed the design. We want to have a stand alone workflow for the bands, not an extension of the relaxation. In any case, if one wants to do relax+bands it should use the code agnostic workchain. Does it matter that internally it separates the steps? However I agree with Sebastiaan that having the possibility to have relaxation with one code and bands with another is an interesting case. So I would try to support if possible, do you believe that the idea I proposed is feasible? |
Only if for a code there is no easy way to actually run it in two steps (or one wants to avoid queuing 2 jobs). But at the moment this does not seem a problem (just pointing out to avoid we discover too late that the design is restrictive Regarding using different codes:
|
That was what was described in the original summary of the design meeting. It says:
This will not be directly possible with the design of this workchain. That is to say, you cannot take the last
The problem here would not just be to make the |
Not only documented, by I will also add a sanity check in the implementation. Thanks @sphuber
You are definitely right... However I do not see any implementation that would allow what we are trying. If I run a relaxation, then the inputs of the input generator are lost anyway. So, even with #235, and being able to translate the Density Matrix, how do I understand what other inputs I should use? Only way is that I wrote somewhere what protocol and so on I used for the relaxation. |
I agree. Just to be clear, I think the design of this PR is the best solution and we should go with it. These comments are more to fully map out what the limitations of the design are and how the various workchains are intended to be used. |
It is a code agnostic workchain that runs first a relaxation and subsequently a bands calculation. It allows the use of seekpath to obtain the path in k space where to calculate the bands (actually this is the default behaviour). In such case an extra scf is run after the relaxation because seekpath might change the structure. The workchain also allows to use one code for relaxation and another for bands. An extra scf step is needed on the relaxed structure with the plugin selected for relaxation. The handling of this case is fragile at the moment and will be improved. The implementation requires to expose the inputs of the input generator for relax and of the input generator for bands. A system has been set up to deal with the fact that a workchain requires Data Nodes as inputs, while the inputs generators accept python types.
|
From my side, and after few interaction with @sphuber, the implementation is now concluded, including the creation of a "code-agnostic" workchain that makes the relaxation first and then calculates the bands. This workchain introduces a new general feature: exposing the inputs of an The implementation does not support yet the possibility to make the relaxation with one code and the bands with another. Also it does not allow "overrides" at the moment. Separate PRs will treat this two issues. But at least this is a usable working implementation. |
zhubonan
left a comment
zhubonan
left a comment
There was a problem hiding this comment.
Thanks for this. Looks good to me!
This PR implements the common bands workchain following the idea expressed by @giovannipizzi in #222.
The main concept is to have the bands common interface that accepts only 3 inputs:
enginesdictionary with the code and resources to use.On top of this we create a code-agnostic workchain called
RelaxAndBandsWorkChainthat runs first a relaxation/scf and subsequently a bands calculation. The use ofseekpathfor the automatic generation of the kpoints is implemented inside this higher level workchain.