Enable direct reading of UPF version >= 2 PP files #980

abussy · 2024-04-04T09:17:16Z

This PR enables the direct reading of standard pseudopotential UPF files with version >= 2, without the need for the upf_to_json tool. The pugixml library is used for this purpose.

All functions reading information from JSON PP files, namely Atom_type::read_pseudo_uspp and Atom_type::read_pseudo_paw have been overloaded with function reading from xml files (stepwise identical). The relevant function is called depending on the file termination in the SIRIUS input file.

This PR is fully backward compatible. At this stage, the dependency on the pugixml library was made a hard requirement. This would imply a change in the spack recipe for the next release.

Edit: at this stage, it is an open question whether pugixml should be a hard requirement or an option.

abussy · 2024-04-08T09:11:02Z

The pugixml dependency was made optional with the addition of a spack variant +pugixml. Whether pugixml is enabled or not, json potential files can be parsed. This PR is fully backward compatible.

Note: for use of pgixmp with CP2K, the CP2K spack recipe (and probably the CP2K toolchain) will need to be adapted. Moreover, the CP2K code will need a slight refactorization, as it currently assumes that SIRIUS only reads JSON files, and it falls back to its own (incomplete) UPF v2 parser otherwise: https://github.com/cp2k/cp2k/blob/2ed94b8b96b2a4590f5340063d84c5d639a6fefe/src/atom_upf.F#L120-L127.

simonpintarelli · 2024-04-08T11:41:26Z

Just as a comment: SSSP currently still contains many UPFv1 pseudos (GRBV), but in the future it will be all UPFv2 🎉 .
The remaining GRBV UPFv1 will be converted to v2 using https://github.com/azadoks/PseudoPotentialIO.jl.

simonpintarelli · 2024-04-08T11:45:02Z

Edit: at this stage, it is an open question whether pugixml should be a hard requirement or an option.

Imho as long as QE parses the UPFs, pugixml should be a variant. The question is if we want to rely on QE to parse UPFs in q-e-sirius or not. At least until a future SSSP is released only containing UPFv2, we should keep it as an optional dependency.

simonpintarelli · 2024-04-08T13:21:19Z

Adding a test under verification would be useful too. E.g. first generate an output_ref.json using the native json format and then run the same input using UPF files directly.

unkcpz · 2024-04-08T23:43:55Z

src/unit_cell/atom_type.cpp

+ if (!nl_node.child("PP_AUGMENTATION").empty()) {
+ for (int i = 0; i < nbf; i++) {
+ std::string istr = "PP_BETA." + std::to_string(i + 1);
+ pugi::xml_node inode = nl_node.child(istr.data());
+ int li = inode.attribute("angular_momentum").as_int();
+
+ for (int j = i; j < nbf; j++) {
+ std::string jstr = "PP_BETA." + std::to_string(j + 1);
+ pugi::xml_node jnode = nl_node.child(jstr.data());


(@simonpintarelli redirect me to this PR. I am maintaining SSSP and working on update SSSP with some improvement to v2)
I think in order to compatible with GBRV pseudos, nqf!=0 should also be supported here. And GBRV pseudos has q_ij rather than q_ijl, https://github.com/QEF/q-e/blob/57a97fe3ac56c6862ee083bf10d27505835c092a/upflib/read_upf_v1.f90#L65-L67

As far as I can tell, GBRV pseudos are stored as UPF version 1 files. We are not aiming to write such a parser. If you are planing to convert to GBRV pseudos to v2, we could cover them as well. But then, I assume they would follow the UPF v2 standard?

But then, I assume they would follow the UPF v2 standard?

Yes, but the UPF v2 standard is for the format, the non-zero nqf and q_ij are still the valid fields according to the UPF v2 schema and according to the notes provided in QEF/qeschemas#11 (comment)

As far as I remember V2 UPF pseudos introduced Q_ij^\ell(r) radial functions as a hard standard. Long term it is better to conver V1 GBRV to V2.

https://gist.github.com/unkcpz/c3287268c84ba46b3366edb5537d65c7

Here is a Pd pseudo I convert from UPF.v1 GBRV to UPF.v2. In the QE, it will then be read by read_upf_new.f90 instead of fallback read by read_upf_v1.f90.
It is true from Giannozzi's note (QEF/qeschemas#11 (comment)) that qfcoef and rinner are to be considered obsolescent. The plan for the SSSP v2 is to provide GBRV with converting it to UPFv2 format (let me know if this is useful, I need more test on such UPFv2 file I converted), I assume in SIRIUS you still can use it? But as @toxa81 pointed, maybe in the long term should push to have use uniformed expression.

I personally think that having the whole SSSP library in UPF v2 format would be very useful, especially for people outside of QE. I also think that having a single uniform standard is important. For example, in this case, we would have to implement a special case for qfcoef and rinner, which in the end, is not very different from having 2 distinct parsers.

As far as I understand, it is possible to go from UPF v1 PP_QIJ to UPF v2 PP_QIJL seamlessly. For example, the upf_to_json tool of SIRIUS parses both UPF v1 and UPF v2 to the same JSON structure, which happens to be very close to that of UPF v2 (PP_QIJL).

For example, the upf_to_json tool of SIRIUS parses both UPF v1 and UPF v2 to the same JSON structure

That is cool, I didn't know it actually convert not only the format but the expression as well. I'll talk to @simonpintarelli to see how it can be realize and I can probably add it to my v1 -> v2 converter.

Just quick update from my side. The converter are ready to convert to PP_QIJL and did exactly the same as what happened in QE internally. So it will be safe with this implementation with the new SSSP 2.0 in the future.

simonpintarelli · 2024-04-09T07:42:25Z

cscs-ci run default

simonpintarelli · 2024-04-10T10:48:32Z

cscs-ci run default

simonpintarelli · 2024-04-10T12:46:36Z

cuda container is failing on non-existent +pugixml variant:
https://gitlab.com/cscs-ci/ci-testing/webhook-ci/mirrors/7415069138667150/4626796664769983/-/jobs/6591482684#L6282

🤔 the cscs-ci container takes the sirius/package.py from upstream spack, while in github-ci we use the package.py from the repository.

simonpintarelli · 2024-04-10T14:59:58Z

cscs-ci run default

(why does it keep disappearing?)

simonpintarelli · 2024-04-10T19:48:25Z

Ping @finkandreas we are running into a timeout when building the cuda base container:
https://gitlab.com/cscs-ci/ci-testing/webhook-ci/mirrors/7415069138667150/4626796664769983/-/jobs/6594225309#L6216

Should we skip it until rosa is back?

finkandreas · 2024-04-11T05:04:28Z

or increase timeout to e.g. 3h, 4h, 5h. No idea how long it takes, but the runner allows long runtimes.
Should be this line: https://github.com/electronic-structure/SIRIUS/blob/develop/ci/cscs-daint.yml#L12

simonpintarelli · 2024-04-11T07:28:35Z

cscs-ci run default

simonpintarelli · 2024-04-11T14:30:56Z

cscs-ci run default

abussy

It's getting out of hand :)

…ure#980) * Enable direct reading of UPF version >= 2 PP files * update spack in cscs-ci * use local spack recipe for cscs-ci * Fix github CI * increase timeout * 4h -> 10h --------- Co-authored-by: Simon Pintarelli <[email protected]>

abussy marked this pull request as draft April 4, 2024 11:30

abussy force-pushed the pugixml branch from 60f2cd9 to b845550 Compare April 4, 2024 13:54

abussy marked this pull request as ready for review April 8, 2024 09:11

unkcpz reviewed Apr 8, 2024

View reviewed changes

Enable direct reading of UPF version >= 2 PP files

fd94def

abussy force-pushed the pugixml branch from d51c4bd to fd94def Compare April 9, 2024 08:37

unkcpz mentioned this pull request Apr 9, 2024

Parser, dumper and converter for USPP azadoks/PseudoPotentialIO.jl#14

Open

1 task

simonpintarelli and others added 3 commits April 10, 2024 14:58

update spack in cscs-ci

19ca80f

use local spack recipe for cscs-ci

c386305

Fix github CI

e4e5b81

increase timeout

00e36bd

4h -> 10h

5fc7539

abussy commented Apr 11, 2024

View reviewed changes

abussy merged commit ef94cd9 into electronic-structure:develop Apr 16, 2024
3 checks passed

simonpintarelli mentioned this pull request Apr 16, 2024

add size check / simplify vec_from_str #983

Merged

abussy mentioned this pull request Apr 18, 2024

Fix test32 #987

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable direct reading of UPF version >= 2 PP files #980

Enable direct reading of UPF version >= 2 PP files #980

abussy commented Apr 4, 2024 •

edited

Loading

abussy commented Apr 8, 2024

simonpintarelli commented Apr 8, 2024

simonpintarelli commented Apr 8, 2024

simonpintarelli commented Apr 8, 2024

unkcpz Apr 8, 2024 •

edited

Loading

abussy Apr 9, 2024 •

edited

Loading

unkcpz Apr 9, 2024

toxa81 Apr 9, 2024

unkcpz Apr 9, 2024 •

edited

Loading

abussy Apr 9, 2024

unkcpz Apr 9, 2024

unkcpz Apr 19, 2024

simonpintarelli commented Apr 9, 2024

simonpintarelli commented Apr 10, 2024

simonpintarelli commented Apr 10, 2024

simonpintarelli commented Apr 10, 2024

simonpintarelli commented Apr 10, 2024

finkandreas commented Apr 11, 2024 •

edited

Loading

simonpintarelli commented Apr 11, 2024

simonpintarelli commented Apr 11, 2024

abussy left a comment

Enable direct reading of UPF version >= 2 PP files #980

Enable direct reading of UPF version >= 2 PP files #980

Conversation

abussy commented Apr 4, 2024 • edited Loading

abussy commented Apr 8, 2024

simonpintarelli commented Apr 8, 2024

simonpintarelli commented Apr 8, 2024

simonpintarelli commented Apr 8, 2024

unkcpz Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

abussy Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

unkcpz Apr 9, 2024

Choose a reason for hiding this comment

toxa81 Apr 9, 2024

Choose a reason for hiding this comment

unkcpz Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

abussy Apr 9, 2024

Choose a reason for hiding this comment

unkcpz Apr 9, 2024

Choose a reason for hiding this comment

unkcpz Apr 19, 2024

Choose a reason for hiding this comment

simonpintarelli commented Apr 9, 2024

simonpintarelli commented Apr 10, 2024

simonpintarelli commented Apr 10, 2024

simonpintarelli commented Apr 10, 2024

simonpintarelli commented Apr 10, 2024

finkandreas commented Apr 11, 2024 • edited Loading

simonpintarelli commented Apr 11, 2024

simonpintarelli commented Apr 11, 2024

abussy left a comment

Choose a reason for hiding this comment

abussy commented Apr 4, 2024 •

edited

Loading

unkcpz Apr 8, 2024 •

edited

Loading

abussy Apr 9, 2024 •

edited

Loading

unkcpz Apr 9, 2024 •

edited

Loading

finkandreas commented Apr 11, 2024 •

edited

Loading