EpwPrepWorkChain: new sub-process epw_bands

[ymzhang0]

I added the new EpwBaseWorkChain as a sub-process with label epw-bands in the EpwPrepWorkChain.

This sub-process shares the same structure as epw sub-processes.

Problem in the protocol files is also fixed. All the metadata namespaces are removed so that option is exposed at the top level.

[mbercx]

Thanks @ymzhang0! Sorry for the slow review. I just have a few small comments, but most changes look great. 👍

[mbercx]

Won't this always be true? Else the first if will already fail, no?

[ymzhang0]

Yes, I'll delete the second if. Actually this way of getting the kpoints for a band calculation is not so good. In W90BandWorkChain run_seekpath is optional upon whether band_kpoints is given in the inputs so I copy the logic here. But we can take the kpoints from the input of wannier90_pp subprocess any way. I'll see if I can fix it.

[mbercx]

Note: I've used ToContext in the past a lot as well, but at some point I found out it's just a dictionary. 😅 You can also do:

Suggested change

	return ToContext(workchain_epw_bands=workchain_node)
	return {'workchain_epw_bands': workchain_node}

And this is exactly the same. I think this is more understandable: you just return a dictionary mapping context keys to processes, and then the AiiDA engine will wait with executing the next step until the processes are completed. No magic ToContext needed. :)

[ymzhang0]

Good point. Now I have a question. How do I deal with that:

return ToContext(epw_interp=append_(workchain_node))

[mbercx]

ToContext is literally just defined as dict:

https://github.com/aiidateam/aiida-core/blame/cd11f08b6a6860181050885e472cd3b04665c743/src/aiida/engine/processes/workchains/context.py#L19

(I remember being quite annoyed when I saw that. No idea why you would make things so complicated, but that's often the case looking at old AiiDA code)

So you should be able to do:

return {'epw_interp': append_(workchain_node)}

But now I wonder if you can't even just literally append to the list in the context? 🤔

Anyways, we shouldn't make these changes for the SuperConWorkChain class in the same commit. I think it's best to keep the scope of a commit minimal, else it becomes difficult to explain why the changes are made. Adding unrelated changes to a commit is a bad idea: it can be confusing to future developers that are trying to understand the code. No need to make a new PR, but let's split up the changes here in multiple commits. As far as I can tell, you are doing multiple things in this PR:

• Fix bugs related to the fact that we put the options at the top level of the EpwBaseWorkChain.
• Add an optional bands interpolation step to the EpwPrepWorkChain.
• Fix a bug in the EpwBaseWorkChain when exposing the inputs for the initial EPW run.
• Update/add help to the EpwPrepWorkChain and SuperConWorkChain inputs
• Avoid using the useless ToContext class.

I wouldn't cram all these in a single commit. If it's ok with you, I can explain/show you how I would split them up once we're ready to merge all the changes.

Just as a note: when working on a PR, try to stay within the scope as much as possible. I also try to do this with my review (although I have not always in the past 🙈). It's great to do extra work of course, but it can make PRs drag on for longer, and then sometimes they never get merged. It's not such an issue here, but something to keep in mind.

[ymzhang0]

Hi Marnik,

I did make several mixed commits that might cause ambiguity in the future. I'll just roll back to the very beginning and make only one commit related to the new bands sub-process. I keep the modification on the always-true elif, more descriptive help but only for the band subprocess without touching the other subprocesses, plain dict as reture value instead of a ToContext one also only for the band subprocess.

For the following changes:

• move options to top level
• write better help message
• use plain dict instead of ToContext

I'll make 3 separate PR later.