[WIP] ABINIT DFPT SHG

[VicTrqt]

Summary

• Different Makers needed for DFPT (DDK, DDE, DTE, Mrgddb)
• A full DFPT SHG workflow to get the SHG and the dielectric tensor

TODO

• Write some tests
• Figure out a way to use the power-ups despite the Response(replace=) for the DDK and DDE
• Is a restart of mrgddb necessary ?

[gpetretto]

Thanks for porting the DFT workflows to atomate. I went thorugh the code and left some comments.

I would say that the main one concerns anaddb. I don't think that

1. anaddb should be run inside the mrgddb job. It should have its own Job and input generator, since it is much more likely to be customized compared to mrgddb.
2. the outputs of the postprocessing with anaddb should be in the mrgddb output document.

An additional point that is not present in the comment concerns the fact that, if I am not wrong, with the introduction of the DFPT workflow the abipy manager should be configured, while before it was not needed. It probably being used to generate the list of pertrubations and to run anaddb behind the curtains. I am wondering if there should be some specific configuration in atomate dedicated to that. Maybe it will be worth to discuss this point more in detail.

[VicTrqt]

I think I have dealt with all the points raised by @gpetretto . I still need to do some cleaning, but it is functional at least. I also haven't done all the proper linting yet, but I'd appreciate any feedback on the points that were raised in the first review. Thanks in advance!

TODO

• Write some tests (I've begun, but not very conclusive because the 1WF files are needed...)
• Is a restart of mrgddb and anaddb necessary?
• Allow for gzip all files (once the whole workflow is done?) Because it becomes very heavy with DFPT and the 1WF files...

[gpetretto]

Thanks @VicTrqt for all the work!
Here are my comments. If needed we can also have a short discussion about them.

[gpetretto]

I think you can remove the "VT". Maybe if someone does not know you the comment might be confusing :P

Also, shouldn't this also happen for the 1DEN files?

[VicTrqt]

not called 1DEN rather DEN1, DEN2, etc... let's leave it at that for the moment and deal with it when the time comes (phonons WF?)

[gpetretto]

This is false. You can make a phonon calculation without DDE and still run mrgddb. This check should probably be removed.

[VicTrqt]

done

[gpetretto]

why is this False by default? Is it because for dte it requires all the DDE even if they can be reduced by symmetry? (I seem to remember this, but I am not sure).
Could it make sense to put the default to None and set the value depending whether dte is enabled or not? Or to True and override in the ShgFlowMaker?

And at the moment, you have the type bool | None. What does None would correspond to? If we don't introduce the option above for None maybe you can remove it from the type?

[VicTrqt]

I removed None and set it to True as default and it is overridden to False in the Shg class.

[gpetretto]

How many values are acceptable of ixc? I don't remember if it is only one or all the LDA are acceptable. In case there are few values acceptable, cannot this be set in the DteMaker or DteSetGenerator with a reasonable default value?

[VicTrqt]

In the end I removed it because it was only affecting the generation of the perturbations, not the calculation in itself. Can be added somewhere in the future if needed.

[gpetretto]

Is there a reason for the code to be like this? in run_rf you declare that the content of prev_outputs should be a list of directories. Here you make a list of lists (thus breaking the API) and then you unfold this in a single list inside run_rf(assuming that under certain conditions the passed values broke the API). Why not making the list here and remove the

    if isinstance(rf_maker, (DdeMaker, DteMaker)):
        prev_outputs = [item for sublist in prev_outputs for item in sublist]

in run_rf?

[VicTrqt]

Following our discussion, I unfolded the list of prev_outputs in the construction of the flow. The main issue here is that .dir_name is a string while ["dirs"] is a list of strings so this means that prev_outputs looks like [dir_name, [dirs]] so I had to create a function to unfold it into a non-nested list with np.hstack.

[gpetretto]

I personally don't like much the AbiBroad name. I suggest something likeBaseAbinitInputGenerator, AbinitInputGeneratorMixin, AbinitToolsInputGenerator.

[VicTrqt]

I changed it to AbinitMixinInputGenerator

[gpetretto]

Can this be removed?

[VicTrqt]

removed

[gpetretto]

can't the code of check_mrgddb_in be directly here? there is only one file to check

[VicTrqt]

moved

[gpetretto]

DDK cannot be reduced by symmetry

[VicTrqt]

removed

[gpetretto]

Maybe also ResponseMaker can be confusing?

[VicTrqt]

see above comment, RespFn?