-
Notifications
You must be signed in to change notification settings - Fork 83
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
Projected density of states #951
base: master
Are you sure you want to change the base?
Conversation
Thanks, looks good! Can you add a test for it? And did you test it with dissociated silicon? |
No problem. Is there a test file for dos or ldos? Then I write the test file with the same format. |
No there's no test right now - it's only covered in chi0.jl. Can you add a new test, dos.jl which tests the DOS, LDOS and PDOS ? |
fix the projected dos bug |
Nice! Can you add an example to examples/, in the same format as the others? |
No problem! |
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.
Thanks very much. Here are some comments.
We also found with @tyjerome that there are indeed additional normalisation issues in the projectors, so they need orthonormalisation before they are fully "correct". For the plots it should not matter as the accuracy we found is 1e-2, but just a note.
@@ -108,4 +108,98 @@ function plot_dos(basis, eigenvalues; εF=nothing, unit=u"hartree", | |||
end | |||
plot_dos(scfres; kwargs...) = plot_dos(scfres.basis, scfres.eigenvalues; scfres.εF, kwargs...) | |||
|
|||
function plot_ldos(basis, eigenvalues, ψ; εF=nothing, unit=u"hartree", |
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.
Is this plot easy to read. I mean it is a 3D thing mapped onto a single axis, indexed in the real-space grid, right ? Would a heatmap or something like this not be better ?
ext/DFTKPlotsExt.jl
Outdated
end | ||
plot_ldos(scfres; kwargs...) = plot_ldos(scfres.basis, scfres.eigenvalues, scfres.ψ; scfres.εF, kwargs...) | ||
|
||
function plot_pdos(p, i::Integer, l::Integer, iatom::Integer, basis, eigenvalues, ψ; εF=nothing, unit=u"hartree", |
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.
To me i
l
and iatom
live in the same category as εF
. Maybe make them kwargs ? For sure in the ordering they should come ofter the basis / eigenvalues and \psi.
ext/DFTKPlotsExt.jl
Outdated
temperature=basis.model.temperature, | ||
smearing=basis.model.smearing, | ||
εrange=default_band_εrange(eigenvalues; εF), n_points=1000, kwargs...) |
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.
can you indent this more to make it clearer that it's a continuation of the kwargs ?
ext/DFTKPlotsExt.jl
Outdated
if l == 0 | ||
return [" "] | ||
elseif l == 1 | ||
return ["y","z","x"] |
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.
spacing around commas
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.
Use LatexStrings ?
ext/DFTKPlotsExt.jl
Outdated
p | ||
end | ||
|
||
function plot_pdos(i::Integer, l::Integer, iatom::Integer, scfres; kwargs...) |
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.
see my previous comment on argument ordering. The scfres
should be the first.
src/postprocess/dos.jl
Outdated
function compute_pdos(ε, i::Integer, l::Integer, | ||
iatom::Integer, basis, eigenvalues, ψ; kwargs...) | ||
compute_pdos(ε, basis, eigenvalues, ψ, i, l, basis.model.atoms[iatom].psp, basis.model.positions[iatom]; kwargs...) | ||
end | ||
|
||
function compute_pdos(i::Integer, l::Integer, iatom::Integer, scfres::NamedTuple; | ||
ε=scfres.εF, kwargs...) | ||
scfres = unfold_bz(scfres) | ||
compute_pdos(ε, i, l, iatom, scfres.basis, scfres.eigenvalues, scfres.ψ; kwargs...) | ||
end |
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.
see previous comments on formatting and argument ordering
src/postprocess/dos.jl
Outdated
pdos = map(x->zeros(typeof(ε[1]),length(ε), 2l + 1), 1:basis.model.n_spin_components) | ||
for j = 1 : 2l+1 | ||
projs_lj = [projsk[:,j] for projsk in projs] | ||
for (i,εi) in enumerate(ε) | ||
pdos_ij = compute_pdos(εi, basis, eigenvalues, projs_lj; kwargs...) | ||
for n_spin = 1 : basis.model.n_spin_components | ||
pdos[n_spin][i,j] = pdos_ij[n_spin] | ||
end | ||
end | ||
end |
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.
needs a formatting pass
After discussion with @xuequan818 : she'll implement a |
@@ -0,0 +1,32 @@ | |||
# # Densities of states (DOS) | |||
# In this example, we'll plot the DOS, local DOS, and projected DOS of Silicon. | |||
# DOS computation only supports finite temperature. |
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.
You should run the SCF computation at zero temperature but the DOSes at finite temperature. Also be careful that the computation time is not too large (haven't checked)
src/postprocess/dos.jl
Outdated
end | ||
D | ||
end | ||
function compute_pdos(ε, basis, eigenvalues, ψ, iatom::Integer; kwargs...) |
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.
Pretty confusing to have all these functions, why not have just this one and the one below?
src/postprocess/dos.jl
Outdated
for (i, ix) in enumerate(ε) | ||
Dx = compute_pdos(ix, basis, eigenvalues, projs; kwargs...) | ||
for σ = 1 : basis.model.n_spin_components | ||
D[i+(σ-1)*length(ε), :] = Dx[σ, :] |
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.
Didn't we say you'd just sum these? Ie D[i,:] += Dx[sigma, :]? Also use views for this
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, I don't consider the spin for ldos. But I see that dos is differentiated by spin, and we plot dos and pdos on a single picture, so I keep spin for the pdos. Should I also sum spin for pdos?
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.
Sure, whatever is the simplest
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.
Got it
src/terms/nonlocal.jl
Outdated
""" | ||
function build_form_factors(psp, G_plus_k::AbstractVector{Vec3{TT}}) where {TT} | ||
abstract type AtomicFunction end |
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.
This is very complicated and not very flexible if we want to add a new type of thing to compute. Just take as input a function, ie atomic_centered_function_form_factors(fun, Gplusk, lmax, n_funs_per_l)
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 will modify these.
No description provided.