New feat: EpwBaseWorkChain

[ymzhang0]

A new EpwBaseWorkChain is constructed based on EpwCalculation. Relative files are also modified:

• aiida entry points in pyproject.toml is updated, the top level key is also set to 'epw' from 'supercon'.
• a new file 'base.py' is add containing EpwBaseWorkChain
• the parts of the EpwWorkChain in epw.py which is previously related to EpwCalculation, including the define(), get_builder_from_protocol() and run_epw() are now adapted for the new EpwBaseWorkChain
• a new protocol yaml is added in protocols/base.yaml, this is the protocol for new EpwBaseWorkChain
• the corresponding epw part of the epw.yaml protocol is substituted by epw_base of the new WorkChain

[mbercx]

Thanks a lot @ymzhang0, great that you made the effort to isolate these changes for a separate PR!

I'll do a more thorough code review later, but wanted to discuss some broad comments first:

1. My main question: should we simply expose the EpwCalculation inputs at the top level of the EpwBaseWorkChain? This is in line with the changes I propose here for aiida-quantumespresso. Now you exclude a bunch of inputs and re-introduce them at the top level. Would it not be easier to just have all inputs at the top level? Curious to hear your opinion here!
2. As you note in one of your TODOs: some of the validation here really belongs on the EpwCalculation. Why not move it there in this PR?
3. A bunch of error handlers seem to be commented? Also other pieces of code seem to be left in a commented state. Is there a reason to leave this code here?

[ymzhang0]

Hello Marnik,

Thanks for the comments.

1. I agree that when we encapsulate the inputs of EpwCalculation into the 'epw' namespace of the BaseRestartWorkChain the structure becomes a bit redundant. In fact I keep the structure only because I'm following the current structure of PwBaseWorkChain since it's more robust than I composing it myself. But would like to simplify it.

2. I try to only add the new base.py file without touching much of the existing code. There are lots of things that should be migrated into the EpwCalculations. I'll then have another commit move it into EpwCalculation maybe after you review the changes?

3. I'll clean the codes commented. Most of them are for better comparison with the previous codes. For the error handlers actually I'm trying to add some handlers for the wrong choice of temperature samplings but didn't really have the time to reproduce the errored calculation. Will also clean them in the next commit.

[mbercx]

Thanks for the comments @ymzhang0! I'll respond point by point:

1. Note that the original reason for adding the "calculation" namespace was to avoid overwriting the metadata of the workchain, see this comment. I've explored if this is an issue somewhat here, and so far I don't think it is. Second, just exposing the inputs at the top-level creates issues for the options, see this comment. In my draft PR for the PwBaseWorkChain, I adapt the port namespace to have the options at the top level of the work chain, see ‼️ Remove pw namespace aiidateam/aiida-quantumespresso#1126.

2. Looking at the validation changes more closely: I see that you added the validation as steps in the outline with corresponding exit codes. I know this is still the approach in some aiida-quantumespresso workflows, but it's an outdated one: having a validator on the input ports is much better since the user gets feedback when they are providing the inputs.

   If it were me, I would open a separate PR that adds the validation to the EpwCalculation you want to add here. In general, I prefer having smaller commits/PRs with a clear and tidy scope, because that makes the changes much easier to understand and review. But let me know if disentangling the changes is too tricky, it might require a bit of git tricks.

3. Best to remove any commented code I think indeed. You can see the old code in the diff as well. :)

[ymzhang0]

OK I've made the changes.

• Comments are removed. If you notice anything left, please let me know.
• EpwCalculation inputs are fully exposed. And related protocols are modified. options are attatched at the top level of EpwBaseWorkChain.
• Validation in EpwBaseWorkChain are temporarily removed. File copying is moved into EpwCalculation. A small validation of parameter is kept in EpwBaseWorkChain.
• EpwWorkChain is renamed as EpwPrepWorkChain as suggested by Samuel.
• I added a small BCS gap function in calculator. It's not related to this commit. If you want I can remove it.

[mbercx]

Thanks for the changes @ymzhang0, much appreciated. I started having a look, but noticed some things that should have been caught by the Ruff formatter/linter. I then realised the CI isn't set up yet. Do you mind if I do this separately, by updating the package template, and then rebase your PR? Then you can see how the pre-commit/CI etc works. Also happy to explain this over a Zoom call.

[ymzhang0]

Hi Manrik,

Please feel free to do so. We can of course schedule a zoom call for that.

[mbercx]

Thanks @ymzhang0! I went through the changes and still left some comments, most of them pretty minor. I didn't fully review the EpwBaseWorkChain yet. I think I still want to change how the options are exposed, but let's switch roles for that one: I'll make the changes based on my draft for the PwBaseWorkChain, and then you can review.

I decided to postpone updating the template; running the linter on the current code base would result in a lot of merge conflicts for this PR, which would be a pain. Let's first integrate your other local changes as well, then we can add the pre-commit etc. and clean things up.

To speed things up: I'll make changes to your branch directly to resolve my comments, and then merge the PR so we can continue working. Let's try to have PRs with a smaller scope in the future, though. :)

EDIT: I've made my suggested changes in ab1c5cf. You can still see the comments in the resolved notes below.