aiidateam / aiida-quantumespresso

The official AiiDA plugin for Quantum ESPRESSO
https://aiida-quantumespresso.readthedocs.io
Other
52 stars 77 forks source link

``PwBaseWorkChain``: improve diagonalization handler #912

Closed bastonero closed 1 year ago

bastonero commented 1 year ago

Fixes #880

The current handler for diagonalization errors is switching from the default value (david) to conjugate gradient (cg). While being a safe solution, the cg algorithm is rather slow compared to other options, now fully supported by the plugin (#901). Therefore, we implement a new strategy, which simply switches to different algorithms before resorting to cg.

mbercx commented 1 year ago

Thanks @bastonero! Just gave the code a quick look, and I'm wondering if it's not a bit over-engineered. Can't we just define e.g. a dictionary that maps the current diagonalization setting to the one we want to use next, where cg is mapped to None that then can be caught to return the ERROR_KNOWN_UNRECOVERABLE_FAILURE exit status? I'm also not sure if the DiagonalizationType Enum adds a lot of value here, because it just corresponds directly to a single input tag for QE.

bastonero commented 1 year ago

I'm wondering if it's not a bit over-engineered.

Yes, I wondered that too, so thanks for the feedback.

Can't we just define e.g. a dictionary that maps the current diagonalization setting to the one we want to use next, where cg is mapped to None that then can be caught to return the ERROR_KNOWN_UNRECOVERABLE_FAILURE exit status?

Yes and no. At the beginning I also thought about this option, but then I wondered - what if one starts directly from e.g. paro and not david? Then one might want to try david as well, but if we set a dictionary with a cyclic call, this won't happen.

Let's imagine we have the following: david -> ppcg -> paro -> cg . Then if paro fails, it will call directly cg, and ppcg and david are not exploited. That's why the logic is more complicated.

I'm also not sure if the DiagonalizationType Enum adds a lot of value here, because it just corresponds directly to a single input tag for QE.

That's true, it might be overcrowded. I just thought maybe we could use it in the future for something, and also it was a nice way to keep track of all the diagonalization options. Moreover, now in QE there are other two diagonalization options (namely 'rmm-davidson' and 'rmm-paro'). So having a DiagonalizationType seemed to me a nice way to have all of them listed. But maybe it is not as necessary.

bastonero commented 1 year ago

@mbercx @sphuber do you have other comments/suggestions? Otherwise, can this be merged for the next release?

mbercx commented 1 year ago

@bastonero My policy is to let the author of a PR write the commit message that will be associated with their account. You should be able to do so since you have "Write" access to the repo. A couple of notes:

  1. IMPORTANT: Since there are several small commits in this PR, be sure to use "Squash and merge" and not "Rebase and merge" and _most definitely not "Create a merge commit". We want a clean, linear and clear history. 😁
  2. Try and respect a line length of 72 characters. This makes logs with git log much easier to read.
  3. Keep the title shorter than 60 characters. Else it won't fit on the first line in GitHub, which is sad. ^^

Screenshot 2023-04-19 at 00 57 23

  1. If it helps, don't hesitate to use commit/PR references. These are picked up by GitHub and make it easy to find related issues in the commit messages.
mbercx commented 1 year ago

Seems you didn't have the permissions after all, but thanks for updating the OP to the desired commit message, @bastonero!