GW-BSE calculation with Abinit #816
GW-BSE calculation with Abinit #816tatha0003 wants to merge 21 commits intomaterialsproject:mainfrom
Conversation
tatha0003
changed the title
gpetretto
left a comment
gpetretto
left a comment
There was a problem hiding this comment.
Thanks @tatha0003 for implementing this.
Here are my comments.
| ) | ||
| 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?
2 participants
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