aiida-quantumespresso
aiida-quantumespresso copied to clipboard
Adding ENVIRON namelists
This PR is in reference to this issue. Added a couple of namelists (boundary and electrostatics) specific to Environ which can be added to the settings dictionary. It is written the same way as the ENVIRON namelist was written - but only called if an ENVIRON key is in the settings dictionary.
Also added an example to test the changes with some typical keys. This example tries to recreate the same calculation as in Example04 with the examples folder of Environ.
Thanks for your contribution @sudarshanv01! Looks great at a first glance, will do a more thorough review later next week when I have time to test this. Two more things:
- Although the example you provide is already great, could you maybe write a short piece of documentation on how to use ENVIRON? I think you can just add it as an extra section to the page on running
pw.x
calculations, i.e. this file in thedocs
. As @giovannipizzi mentioned, this isn't currently documented anywhere, so would be good if we could add this. Note that the documentation still needs a lot of work, that'll be my main focus after the tutorial has finished. :) - I'm not sure how familiar you are with writing tests, but I think it would be good to add a test for these namelists to the tests for the
PwCalculation
, see this file. Let me know if you need help with this, and I'd also be happy to add a test myself once I'm done reviewing the code.
As a sidenote: does ENVIRON produce any new output in the stdout? Or does it only produce extra files with AiiDA then has to retrieve?
Absolutely - will write a section on ENVIRON and will add tests!
That is a good point, about the stdout. There are a few quantities that could be potentially interesting to parse from stdout - I imagine that would go into the parser - I can take a look at adding those in as well!
Absolutely - will write a section on ENVIRON and will add tests!
Great, thanks a lot! Let me know if you need any help. 🙃
That is a good point, about the stdout. There are a few quantities that could be potentially interesting to parse from stdout - I imagine that would go into the parser - I can take a look at adding those in as well!
Just a note: In general, it's better not to parse from the stdout in case there is an alternative that is less likely to break. For example in the pw.x
parsing, we try to get as much information from the data-file-schema.xml
, since we als have the XSD schemas for the different versions of QE, so this will not break when changes are made by the QE developers, since we can simply load the schema of the correct version. If there is something like this for ENVIRON, it would be better than parsing information from the stdout, since any change in formatting can break the parser.
@sudarshanv01 just a quick ping on this: will you still have time to update the PR based on @sphuber's suggestions? If not, that's fine, I'd happily take over to fix the tests so we can get this baby merged.
@matthew-truscott just pinging you so you are aware of this proposed change by @sudarshanv01. Would the EnvPwCalculation
still be needed once this PR is merged?
Thanks for the mention, and good to see more support for Environ with AiiDA. EnvPwCalculation
in its current state does not provide much more functionality (there is an update to the github mirror scheduled soon where we introduce datatypes to handle specific Environ charge objects that can be added to the input file as cards, and parsing options for the debug file that is generated by Environ). We also plan on adding exit codes and logging messages that reflect various environ specifc issues that can arise in a PW self consistent calculation.
Environ itself is getting some pretty significant updates that will warrant a completely separate calcfunction, in particular, Environ will act less like a plugin on top of QE and more like a standalone code that leverages the QE drivers, among other things.