Structure of the Espresso recipes

[Andrew-S-Rosen]

@tomdemeyere: Before proceeding with #1404, I wanted to take a moment to clean up some of the existing Espresso content, which I did in #1426 and #1427 (now both in main).

When you get a moment, could you review main and let me know if there are any changes/issues with the current implementation? I want to stress that if there are any major problems, I can revert the content in the commits as needed. I am happy to discuss the motivation for the various changes, most of which are relatively minor.

Note: I will handle all merge conflicts that arise in #1404.

[tomdemeyere]

For #1426 I am not sure about number 5. I think you should make your point on the ASE repo but, if ASE accept both then quacc should accept both? Additionally calling construct_namelist 2 or 3 times to change the flat form to the nested form is not ugly enough to justify additional confusion/annoyance to the user in my opinion.

I've set myself a rule that is: everything that is internal to quacc is always done in the nested form (preset, defaults...). Thus, the only time we have to call construct_namelist is on user provided data. We could actually think about tests that enforce this.

[Andrew-S-Rosen]

Thanks for your input! I am not opposed to adding that content back in and will do so if you feel it worthwhile enough.

I've set myself a rule that is: everything that is internal to quacc is always done in the nested form (preset, defaults...). Thus, the only time we have to call construct_namelist is on user provided data.

Ah, I noticed that. I guess my question is can we ensure that that's always the case? A user might make their own preset with the flat form, or a new Espresso contributor might make the defaults in flat form. So, it seems a bit difficult to control. Since everything in quacc was done in nested form, I figured let's go all-in. But, if that is an inconvenience on the user, the solution will be to just call the construct_namelist anywhere necessary. I'm not opposed. To me, it seemed like a more fragile approach because if new input flags are added in a new version of Espresso, they may be silently ignored when using the flat approach.

[tomdemeyere]

I guess my plan was to form some kind of perfect barriers by putting construct_namelist on any user data (this is also useful to go to the case-insensitive world, although a little overkill). It just occurred to me that users can also define their own presets... In this case we just need to put yet another construct_namelist just after preset reading (It does not do anything if the dictionary is already nested, just putting everything to the Namelist class)

But you are also right about the silently ignored keywords... Funnily enough I use this feature when quickly writing input files to check if my keywords do not have typos, but I guess that's not the intended use case!

There are a lot of good sides and bad sides, I actually thought about that for a while when I started this quacc interface, but in the end I decided to go for the full compatibility with ASE at the expense of a slightly more messy code. If anything to convince you: as a heavy ASE espresso user, if the quacc main branch does not contains the flat compatibility, the first thing I would do is to fork it and add it... I know a few people that would surely do the same.

[Andrew-S-Rosen]

All very useful input, thank you for taking the time! I truly appreciate it.

If anything to convince you: as a heavy ASE espresso user, if the quacc main branch does not contains the flat compatibility, the first thing I would do is to fork it and add it... I know a few people that would surely do the same.

Perfect. This is the kind of feedback I was hoping to get. That is enough to convince me that it is worthwhile to consider. I pointed out some inconsistencies, which also existed in the previous implementation, in #1429. If we can solve those, then it should be okay to re-add.

[tomdemeyere]

There were a few instances before where the user-specified parameters were overwritten by a preset, which is a strict no-no. We should always respect the user's parameters. This was most notable with things like ecutwfc. If the user specified a value of 50, but the preset found a maximum elemental ecutwfc of 100, it'd overwrite the user's 50 value. In practice, if the user asks for a value, they should see that value in the input file. In practice, if a user wants to rely on the max value from the preset (typically the case), they should not explicitly specify an ecutwfc at all.

I agree, that's a strict no-no, this was never the philosophy I aimed for, this should have been covered by the tests, thanks for spotting that

[Andrew-S-Rosen]

Got it. Perhaps it was me misinterpreting something. Either way, it's all good now!

[tomdemeyere]

Several aspects of the calculator were internally refactored without any change of functionality to the recipes. For instance, see the rewrite of parse_pw_preset to parse_pseudopotential_info.

Much cleaner, I didn't think about merge_dict taking more than 2 parameters

[tomdemeyere]

I merged in the changes from #1415, which moves the phonon_job to its own module and makes the previous directory a required argument to better inform the user what is needed.

At the beginning I wanted to make sure users understand which espresso binary is used in this phonon job, phonons can be done in many ways (dfpt, fd, md, ...), that's why I started using 'ph' for ph.x jobs. Eventually I got lost in the work but if you are happy about how it is now I have nothing more to say

[tomdemeyere]

@Andrew-S-Rosen Please have a look at: https://gitlab.com/ase/ase/-/merge_requests/3199 This should help to do some important clean up in quacc namely, all the keys stuff