GW-BSE calculation with Abinit #816
Conversation
tatha0003
changed the title
There was a problem hiding this comment.
Thanks @tatha0003 for implementing this.
Here are my comments.
| @@ -77,7 +77,7 @@ class NonSCFSetGenerator(AbinitInputGenerator): | |||
| factory: Callable = nscf_from_gsinput | |||
| pseudos: str | list[str] | PseudoTable | None = None | |||
| restart_from_deps: tuple = (f"{NSCF}:WFK",) | |||
| prev_outputs_deps: tuple = (f"{SCF}:DEN",) | |||
| prev_outputs_deps: tuple = (f"{SCF}:DEN", f"{SCF}:WFK",) | |||
There was a problem hiding this comment.
This will always require that the WFK is present when doing a non-scf calculation, but this is not always the case. I am afraid this could make flows like the baand structure fail. If you need to have this maybe it can be set specifically for the Flows where it is needed?
However, it does not seem that the NonSCFSetGenerator is explicitly used in any of the workflows. Is this needed at all?
There was a problem hiding this comment.
Yes, you are right, this is not necessary. I have removed it.
| ) | ||
| ab1 = load_abinit_input(prev_outputs[0]) | ||
| if NSCF not in ab1.runlevel: | ||
| raise RuntimeError("Could not find one NSCF calculation.") |
There was a problem hiding this comment.
In principle all the checks above should be already be performed in the get_abinit_input in the base class, since the factory_prev_inputs_kwargs is set. This does provide more detailed error messages, but I would rather improve the errors provided by the base class, rather than overriding the method for all the subclasses. I think that all the implementation of this method can be removed, is that correct?
| else: | ||
| raise RuntimeError("Could not find one NSCF and one SCREENING calculation.") | ||
|
|
||
| if nscf_inp.vars["ngkpt"]!=scr_inp.vars["ngkpt"]: |
There was a problem hiding this comment.
As in the case of BSEmdfSetGenerator, also here most of the checks should be provided in the base class by the fact that factory_prev_inputs_kwargs is set.
This check on ngkpt is additional. I understand that this might be important, but is it really ngkpt the only value that would make the calculations incompatible? What if other keys as ecut or shiftk are different, just to mention a few?
If needed, I think this check can be performed in the factory, so the implementation of get_abinit_input could be removed entirely from here.
| if factory_kwargs: | ||
| factory_kwargs.update({"ecutwfn": nscf_inp["ecut"]}) | ||
| else: | ||
| factory_kwargs={"ecutwfn": nscf_inp["ecut"]} |
There was a problem hiding this comment.
Can this be done in the factory? Or is it not strictly necessary in general?
| ) | ||
|
|
||
| class Config: | ||
| arbitrary_types_allowed = True |
| shifts=self.shifts, | ||
| chksymbreak=0) | ||
| ) | ||
| nscf_job = update_user_abinit_settings( |
There was a problem hiding this comment.
If there are several updates that need to be performed on the nscf input, could it be worth to define a specific input set?
| return Flow(jobs, output=bse_job.output, name=self.name) | ||
|
|
||
| @job(name="Find BSE parameters") | ||
| def find_bse_params(self, bandlims, enwinbse, directgap): |
There was a problem hiding this comment.
As for the other Flows, I think it would be better if the jobs are not methods of the Maker, aside from make. This could simply be a job function in jobs/bse.
| # - check nbands in nscf is >= nband in screening and sigma | ||
| # pass | ||
|
|
||
| def make( |
There was a problem hiding this comment.
Can't this simply be a subclass of ConvergenceMaker redefining the default value that change (e.g. criterion_name, epsilon, ...)?
|
|
||
| name: str = "BSE Mutiple Shifted Grid" | ||
| scf_maker: BaseAbinitMaker = field(default_factory=StaticMaker) | ||
| bse_maker: BaseAbinitMaker = field(default_factory=BSEFlowMaker) |
There was a problem hiding this comment.
As above, this is declared as a BaseAbinitMaker, but the default is a BSEFlowMaker. Should the type be just a Maker?
| name: str = "BSE Mutiple Shifted Grid" | ||
| scf_maker: BaseAbinitMaker = field(default_factory=StaticMaker) | ||
| bse_maker: BaseAbinitMaker = field(default_factory=BSEFlowMaker) | ||
| shiftks: list = None |
There was a problem hiding this comment.
From the code seems that shiftks cannot be None. It would be better to remove the default value, so that it is clear that the value should be provided.
Summary
Include a summary of major changes in bullet points:
Additional dependencies introduced (if any)
No additional dependencies
TODO (if any)
Need to move /atomate2/abinit/sets/factories.py to abipy/abio/factories.py