-
Notifications
You must be signed in to change notification settings - Fork 87
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for NC pseudos in UPF format #741
Conversation
If the rab are already the integration weights, why do you do a quadrature (ctrap) rather than just a dot product? |
I wasn't sure if they account for the half-weight at zero. I did a bit of testing comparing the dot product and the ctrap with the half-weight, and they seem to be in similar agreement with HGH. I've changed to using a dot product, it's probably a good step for better performance assuming that it's the right thing to do. |
It looks like ~78% of time in initializing the Allocations could still be improved, function init_si(pseudo_type, n)
a = 10.26 # Silicon lattice constant in Bohr
lattice = a / 2 * [[0 1 1.];
[1 0 1.];
[1 1 0.]]
Si = ElementPsp(:Si, psp=load_psp("$pseudo_type/pbe/si-q4.$pseudo_type"))
atoms = [Si, Si]
positions = [ones(3)/8, -ones(3)/8]
model = model_PBE(lattice, atoms, positions)
for _ = 1:n-1
PlaneWaveBasis(model; Ecut=15, kgrid=[4, 4, 4])
end
PlaneWaveBasis(model; Ecut=15, kgrid=[4, 4, 4])
end
basis = @btime init_si("upf", 1); # benchmark
6.753 s (26559047 allocations: 865.72 MiB) Initializing the |
If the profiling clearly shows up the besselj function, working on allocations isn't going to do anything. |
With judicious use of |
Just did some quick tests for integrating an array of size 100000 with l=2. So no need to go do complicated optimizations, it looks like :-) Here's the final version:
|
Sorry, there was a mistake in the previous post (if you read this via email notifications), updated now. |
Ok sweet, I did write up the hand-coded spherical Bessels, which was already much faster. I thought that I'll work a bit on implementing like this with |
# For Silicon, 2-atom cell, si-q4.upf
Ecut = 80
kgrid = [12, 12, 12]
julia> basis = @benchmark PlaneWaveBasis(model; Ecut=Ecut, kgrid=kgrid) # benchmark
BenchmarkTools.Trial: 1 sample with 1 evaluation.
Single result which took 32.746 s (0.65% GC) to evaluate,
with a memory estimate of 1.41 GiB, over 3165129 allocations. Much better! |
Still looks expensive but combined with q interpolation it should be ok. You can avoid the code duplication by using Val types and function barriers: have order be a Val type, and convert from int to Val in a separate function from the loop (so that the loop is compiled using the static type information for the order, and propagates it to the inlined bessel function). There are instances of such tricks in the "performance tips" section of the julia manual |
I think this is as you suggest(?), but it seems to break @inline function sphericalbesselj(order::Val{0}, x)
sin(x) / x
end
@inline function sphericalbesselj(order::Val{1}, x)
(sin(x) / x - cos(x)) / x
end
...
function psp_projector_fouier_inner_loop(order::Val, psp::PspUpf, i, l, q::T)::T where {T <: Real}
N = length(psp.rprojs[l+1][i])
s = zero(T)
@turbo for ir = 1:N
s += sphericalbesselj(l_val, q * psp.rgrid[ir]) * psp.rprojs[l+1][i][ir] * psp.drgrid[ir]
end
end
function eval_psp_projector_fourier(psp::PspUpf, i, l, q::T)::T where {T <: Real}
if iszero(q)
if l == 0
s = T(1)
else
s = T(0)
end
else
l_val = Val(l)
s = psp_projector_fouier_inner_loop(l_val, psp, i, l, q)
end
return s
end
julia> PlaneWaveBasis(model; Ecut=Ecut, kgrid=kgrid); # warm up
┌ Warning: #= /data/azadoks/source/git/DFTK.jl/src/pseudo/PspUpf.jl:172 =#:
│ `LoopVectorization.check_args` on your inputs failed; running fallback `@inbounds @fastmath` loop instead.
│ Use `warn_check_args=false`, e.g. `@turbo warn_check_args=false ...`, to disable this warning. |
Hm curious (and annoying). That looks like a limitation of LoopVectorization, I'm not sure how to work around it... that said if just by using hand-coded and SIMD we get reasonable enough performance, we might just as well not use LV... |
It seems like # For Silicon, 2-atom cell, si-q4.upf
Ecut = 80
kgrid = [12, 12, 12]
julia> basis = @time PlaneWaveBasis(model; Ecut=Ecut, kgrid=kgrid); # benchmark
70.301417 seconds (29.15 M allocations: 2.576 GiB, 0.61% gc time, 0.08% compilation time) With > # For Silicon, 2-atom cell, si-q4.upf
> Ecut = 80
> kgrid = [12, 12, 12]
julia> basis = @time PlaneWaveBasis(model; Ecut=Ecut, kgrid=kgrid); # benchmark
69.722348 seconds (29.08 M allocations: 2.573 GiB, 0.43% gc time) A glance through the
|
Dftk does pride itself on simplicity so I'd say let's forget about LV, just use simd and Val. The timings you report are reasonable I'd say. In any event, if we want to get more perf, we just do interpolation in q space (which even with a very fine mesh should result in an extremely significant boost, since we are basically sampling all of 3d q space here, and the functions should be very smooth functions of q) |
Also LV is kind of in flux and due for a rewrite from what I understood, so hopefully in one year or so we just slap turbo on the Val version and it'll just work |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some notes.
Another thing that could be worth trying is to parallelise over the evaluation of projectors or the evaluation of Fourier components in the local potential. This will certainly help for larger calculations on clusters. |
I agree with Antoine regarding the tradeoff simplicity / speed. I think what we have now is ok in speed and I would avoid making the code more complicated if possible. |
On that point: I think we should certainly not try to have a comprehensive UPF pseudo library in our code base. I see two options:
I lean towards the first thing, but I don't have an overview how many pseudo libraries are actually used, i.e. if there is a combinatorial explosion sitting behind this solution that one should better not buy into. |
Also: I think there are still some stability issues. Just out of curiosity I tried a UPF for a pretty big atom (Hafnium) and got greeted with NaNs. I have not investigated further as I think we should slowly make our way up the complexity ladder, but just to warn everyone :). |
Agreed we should not have these in the repo. Gth are fine because they are super small fine, but beyond that it's fine to require people to get the upf themselves (and having a repo with many of them is a separate project) |
I suspect this is because it's on a linear grid (is it from ONCVPSP?), and the linear grids usually contain |
Sounds good. It could be nice eventually to have something à la Shall we leave in a few pseudos for testing purposes? Maybe There might be some other pseudo generators that have their own quirks for which we could have an example for testing; I should probably look into this... |
I made a bit of a mess yesterday trying to rebase, but I've fixed up a few things:
What do you guys think are the remaining key points to address to wrap this all up? |
Re
I'm not sure we should have them in
From my point of view there are two blocking things:
A nice example could also be good (I guess you can just download the UPF file from e.g. https://pseudopotentials.quantum-espresso.org/legacy_tables/hartwigesen-goedecker-hutter-pp/si in the test and do the SCF once with the UPF and once with the analytical HGH? |
I agree, but
True, I'll move them into
Ah yeah, I think this will definitely take some careful thought to try to get all the inputs lined up as well as we can. I'll give it a first shot and share the input files. |
Oh yes, this is tricky. Don't try to be too fancy at first (i.e. use a simple functional and setup, first one projector then many etc). But it's worth doing. I found quite a few smaller errors along the way when I did this with ABINIT back in the days. |
Yeah, this is quite messy.
Because the units of For the local potential, I don't understand why, but if I convert before the FT, |
I've just committed a (non-working) draft of the example: the idea is to run an Ecut convergence on an HGH and a PseudoDojo UPF pseudo (to show the difference in how hard they are). Then, use the converged Ecut to plot a bandstructure (hopefully to show that the results are quite similar). I chose Magnesium because it's one of the few heavier elements (i.e. not H, He, Li) in PseudoDojo that doesn't contain NLCC (which we don't yet support). I can't seem to get the 'mg-q10.hgh' pseudo to converge (up to 76 Ha, I haven't tried higher). |
Hmm. I think this is too large an Ecut for the docs to handle in the CI. E.g. for https://docs.dftk.org/stable/examples/convergence_study/ we only run rather moderate Ecuts on the CI and show static images for the more reasonable sizes. Regarding the large Ecuts: I'm not super surprised as HGHs are indeed quite hard. I also know other cases, where we had to go to beyond 100 to converge to 4-5 digits. I'm not sure it's necessary to do a full convergence + bandstructure example here. I would literally just show how to obtain the pseudos (for now just download from the git repo, I guess) and then run an SCF at a moderate ecut. Surely the results will not be 100% the same, but that's fine. |
I guess this is probably more reasonable; I still think it's interesting to show the convergence, so I could do that with static images. Running the SCF won't really show anything because the energies will be quite different. Something like a bandstructure or equation of state should show that the pseudos give results that agree, at least somewhat. |
Makes sense. In any case: If you create static images, please always add the code to generate the images to the https://github.com/JuliaMolSim/DFTK.jl/blob/master/docs/src/assets/0_pregenerate.jl file to ensure we can easily update them. |
docs/src/assets/0_pregenerate.jl
Outdated
plot!(plt, conv_hgh.Ecuts, conv_hgh.errors, label="HGH") | ||
plot!(plt, conv_upf.Ecuts, conv_upf.errors, label="PseudoDojo UPF") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Show convergence on a logplot, thicker lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very fishy, why is there such a big plateau? Is it because of the tolerance? If so can you just not display that part of the curve?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise very interesting! I knew the hgh were kind of hard but the difference is huge!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😱 That does look like a numerical grid issue. Do you by any chance have a graph of |V(q)| and |beta(q)| in semilog scale at the ready (so we can see at which rate it decays)? Otherwise I can take a look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can probably get it in a few mins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, it's a grid issue. I'm opening a separate issue, it shouldn't hold back the merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note the potential seems stagnant around q=10, which is around Ecut=50, so that's consistent.
docs/src/assets/0_pregenerate.jl
Outdated
plot!(plt, conv_hgh.Ecuts, conv_hgh.errors, label="HGH") | ||
plot!(plt, conv_upf.Ecuts, conv_upf.errors, label="PseudoDojo UPF") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it's the SCF tolerance. By default in this example tol/100
where tol
is the convergence we are aiming for (so 1e-3).
Apart from the cosmetic nits on the plot script and the pregenerated plot this LGTM. |
@azadoks Great PR, thanks! |
Head branch was pushed to by a user without write access
Based on @unkcpz 's PR.
To do:
PP_RHO
)PP_CHI
)|G|
?PseudoPotentialIO
quadgk
references used for HGH don't agree well with UPFs (Needed correction term)