search

aiidateam / aiida-wannier90-workflows

A collection of advanced automated workflows to compute Wannier functions using AiiDA and the Wannier90 code
http://aiida-wannier90-workflows.readthedocs.io/
Other
16 stars 17 forks source link

Simplify `get_builder_from_protocol` in `ProjwfcBandsWorkChain` #56

Closed t-reents closed 2 months ago

t-reents commented 4 months ago

Hi @qiaojunfeng, thanks for this nice work!

We (well, mainly @AndresOrtegaGuerrero) are working on integrating the ProjwfcBandsWorkChain into AiiDAlab. Andres realized that it's not possible to provide Python datatypes in the overrides when using the get_builder_from_protocol method (I could confirm this behavior). One example is passing a float for kpoints_distance. However, this is possible in the AiiDA-QE plugin.

The problem occurs in the following part: https://github.com/aiidateam/aiida-wannier90-workflows/blob/d6794cf44a3ab3d6ac11fcfff06a57603e824480/src/aiida_wannier90_workflows/workflows/projwfcbands.py#L157

The PwBandsWorkChain.get_builder_from_protocol transformed the relevant inputs into AiiDA datatypes, but the aforementioned call of recursive_merge_container merges the plain Python datatypes from the protcol_inputs again.

I fixed this behavior in this PR and generally tried to simplify the get_builder_from_protocol. As the ProjwfcBandsWorkChain is a child class of the PwBandsWorkChain, the majority of the preparation is already done in PwBandsWorkChain.get_builder_from_protocol. For example, I'm not sure if the following is really needed: https://github.com/aiidateam/aiida-wannier90-workflows/blob/d6794cf44a3ab3d6ac11fcfff06a57603e824480/src/aiida_wannier90_workflows/workflows/projwfcbands.py#L122-L124

The protocol inputs are anyway passed to the get_builder_from_protocol methods of the sub-workchains and the ProjwfcBandsWorkChain doesn't have any additional inputs that are specific to this WorkChain.

I tested the changes and the results seem to be identical (at least within my tests). In case I removed parts that are necessary for the intended functionality because I misunderstood them, please let me know. I'm happy to revert certain changes/to adapt them.

aiida-cla-bot[bot] commented 4 months ago

All contributors have accepted the CLA ✅

You might need to click the "Update/Rebase branch" button to update the pull request and rerun the GitHub actions to pass the CLA check.
Posted by the CLA Assistant Lite bot.

t-reents commented 4 months ago

I have read the CLA Document and I hereby accept the CLA

AndresOrtegaGuerrero commented 3 months ago

@t-reents could you update the branch and also solve the issue with the CLA Assistant error ? , I already tested the branch and it works.

t-reents commented 2 months ago

As a general comment, we realized that the issue would also be solved by upgrading to aiida-core==2.6.2 (@AndresOrtegaGuerrero encountered it in aiida-core==2.5.1). There it is possible to pass the Python base types to the builder. However, it is not clear when aiidalab-qe will be upgraded to that aiida-core version, and therefore, the changes in this PR are still useful.