`XspectraCoreWorkChain`: Fix Oversight in `get_builder_from_protocol()`

[PNOGillespie]

This PR fixes an oversight in the get_builder_from_protocol() method of the XspectraCoreWorkChain where not all the overrides to the scf input namespace were being applied correctly.

Previously, overrides were applied to the pw.parameters part of scf correctly when the parameters required for the chosen core-hole treatment were set in the method, however overrides for the rest of the scf namespace were erroneously omitted in the call to PwBaseWorkChain.get_builder_from_method() in the previous step. In this fix we simply address this oversight.

[sphuber]

On another note, I am not sure how you are creating your feature branches, but it seems you are creating them from a branch that is outdated and contains a lot of old commits. Every time you have a PR there are a ton of non-related commits.

To fix this, you can do the following from your local clone:

git fetch --all
git checkout develop
git reset --hard origin/develop  # Here I assume that the `aiidateam/aiida-quantumespresso` remote is called `origin`. Use `git remote -v` to get the names of the remotes and change `origin` with the correct name if need be

This essentially resets your local develop branch to the state of the main repo.

Now, if you want to create a new feature branch, you simply do

git checkout -b feature/branch develop

But you don't even need the local develop checked out. Simply do

git fetch --all
git checkout origin/develop -b feature/branch

This ensure that your branch is up to date and make the PRs easier to understand without all the superfluous commits.

[PNOGillespie]

Hi @sphuber, thanks for the review. Regarding the regression test (just so it's clear to me): this would just be to test that the overrides outside of pw.parameters in the scf namespace (such as kpoints_distance) are applied correctly?

On another note, I am not sure how you are creating your feature branches, but it seems you are creating them from a branch that is outdated and contains a lot of old commits. Every time you have a PR there are a ton of non-related commits.

To fix this, you can do the following from your local clone:

git fetch --all
git checkout develop
git reset --hard origin/develop  # Here I assume that the `aiidateam/aiida-quantumespresso` remote is called `origin`. Use `git remote -v` to get the names of the remotes and change `origin` with the correct name if need be

This essentially resets your local develop branch to the state of the main repo.

Now, if you want to create a new feature branch, you simply do

git checkout -b feature/branch develop

But you don't even need the local develop checked out. Simply do

git fetch --all
git checkout origin/develop -b feature/branch

This ensure that your branch is up to date and make the PRs easier to understand without all the superfluous commits.

I wasn't aware this was something one could do with Git, but I will do this in the future. Can it be applied to the branch for this PR now, or is it too late for that?

Edit: I've been keeping my local branch up to date via my Fork of the repository, rather than the main repository itself (essentially pulling updates to the Fork develop branch, then pulling to the local develop branch), so maybe that's what the issue is? Probably this can be fixed just by targeting git reset to upstream (aiidateam/aiida-quantumespresso), but you likely know more than I do on that.

[PNOGillespie]

Hi @sphuber. I have added a test to tests/workchains/protocols/xspectra/test_core.py to check the overrides. The test checks that the overrides work for both scf.pw and scf.pw.parameters. Since get_builder_from_protocol() sets SCF parameters required for a given core-hole treatment option but should also allow the overrides to take precedence, I realised that there should be a check in place to confirm that this works.

[sphuber]

I wasn't aware this was something one could do with Git, but I will do this in the future. Can it be applied to the branch for this PR now, or is it too late for that?

Could also be done, although not necessay. It would be something like:

git checkout fix/xspectra-core-workchain
git fetch --all  # Make sure you are up to date
git reset --soft origin/main  # This will basically reset the branch to `origin/main` but keep the changes of your feature branch locally
git commit # Commit the changes again
# Here maybe check with `git show -1` that the latest commit contains all the changes of the branch and nothing was lost
git push --force fork fix/xspectra-core-workchain  # Force push the new branch, which should now have just 1 commit on top of `aiidateam/main`

Edit: I've been keeping my local branch up to date via my Fork of the repository, rather than the main repository itself (essentially pulling updates to the Fork develop branch, then pulling to the local develop branch), so maybe that's what the issue is? Probably this can be fixed just by targeting git reset to upstream (aiidateam/aiida-quantumespresso), but you likely know more than I do on that.

That is very likely it. To be honest, there is no real need to keep your fork's main branch up to date as you are never really using it (should simply be a clone of aiidateam/main). So indeed, I would just always clone from aiidateam/main and forget about PNOGillespie/main.

You don't really ever even have to check out the main branch locally. If you want to create a new branch, you simply do:

git fetch --all
git checkout origin/main -b some-branch-name-to-create

And that's it. Now you have a new branch that is up to date with the latest main branch.

[sphuber]

Regarding the regression test (just so it's clear to me): this would just be to test that the overrides outside of pw.parameters in the scf namespace (such as kpoints_distance) are applied correctly?

Indeed. From a first glance, that is what your added test seems to be doing, so all good.