Skip to content
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

Rewrite Wannier interface #854

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

qiaojunfeng
Copy link
Contributor

Finally, it's here! 🚀
Now I have finished the rewriting of Wannier function interface, including

  • use WannierIO.jl for reading/writing Wannier90 files
  • In addition to the current wannier90_jll example, added an example of using Wannier.jl to construct Wannier functions
  • For initial projections, in addition to the current random Gaussain initial guess, added
    • Hydrogenic orbital (i.e., analytic solution of radial function) projection with different angular momentum
    • Pseudized-atomic orbitals (from pseudopotentials) projection, currently only for PspUpf since it contains the pswfcs field
      • this should also enable us to compute PDOS, projected bands, and DFT+U in the future
    • SCDM projection (QR factorization of wavefunctions)
  • support :collinear calculation

Still have to add some more tests, but maybe now we can discuss the file/function namings, design, etc.

@qiaojunfeng
Copy link
Contributor Author

Oh it miserably failed since my packages are not registered yet, it's ongoing qiaojunfeng/WannierIO.jl#1

@antoine-levitt
Copy link
Member

Oh wow that's awesome, thanks so much for doing this! Give me a couple days and I'll review. In the meantime @LaurentVidal95 can you maybe take a look?

@LaurentVidal95
Copy link
Contributor

Super nice ! I'll give it a look tomorrow with pleasure 👍

@mfherbst
Copy link
Member

Very nice. Thanks for the PR @qiaojunfeng. I'll make a pass as well within the next few days.

Copy link
Member

@mfherbst mfherbst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks very much. Some comments from my end.

Comment on lines +2 to +3
#
# DFTK features an interface with the program
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid code duplication here, can't we just merge it with the wannier90 example? In fact I would even suggest to make this the main example and then just add a small section, that show how one could do the same thing with wannier90, but explaining the details a second time.

Comment on lines +59 to +60
exclude_bands = DFTK._default_exclude_bands(scfres)
basis, ψ, eigenvalues = DFTK.unfold_scfres_wannier(scfres, exclude_bands)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to give the argument if it's the default parameter I would say. That's why we have these default_ functions in the code. Also we usually don't prefix them _default_, but just default_

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(BTW I'm reading this top to bottom, so some of my comments might better apply to the wannierio.jl files)

Comment on lines +67 to +68
μ, σ = 0.0, 0.01
f = DFTK.scdm_f_erfc(basis, eigenvalues, μ, σ)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you make \mu and \sigma kwargs, then this is obvious without the extra line assigning \mu and sigma

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should bikeshed these names ... scdm_f_erfc is quite cryptic to someone who does not already know wannier90.

Comment on lines +70 to +71
n_wann = 5
A = DFTK.compute_amn_scdm(basis, ψ, n_wann, f)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also better spell it out n_wannier

fileprefix="wannier/graphene",
n_wann,
A,
dis_froz_max=0.1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you use these cryptic keyword arguments for compatability with wannier90, but I'm really not a fan of this. Can we not avoid that somehow and give it more descriptive names?

If not: at least add a comment here.

- `α`: diffusivity, ``\frac{Z}/{a}`` where ``Z`` is the atomic number and
``a`` is the Bohr radius.
"""
function radial_hydrogenic(r::AbstractVector{T}, n::Integer, α::Real=1.0) where {T<:Real}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these hydrogenic things should live in common or maybe even in pspio. They sound like the could be useful for other things, too.


for (ik, εk) in enumerate(eigs)
for (n, εnk) in enumerate(εk)
E[n, ik] = auconvert(Unitful.eV, εnk).val
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better use ustrip.

Comment on lines +563 to +566
function unfold_scfres_wannier(
scfres::NamedTuple,
exclude_bands::Union{AbstractArray{<:Integer},Nothing}=nothing,
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LaurentVidal95 Didn't you write a function at some point that does more or less this?

In principle, one should use better initial guesses, e.g.,
[`guess_amn_hydrogenic`](@ref), [`guess_amn_psp`](@ref).
"""
function save_wannier(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to run Wannier.jl without saving these?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I don't like this monolithic function. I think it's much better to have individual functions to write these files (e.g. by providing DFTK-specific methods to WannierIO.write_amn etc.) and then the user just has to call them in the right order and supplying the right guess methods etc.

Comment on lines +689 to +693
function _default_exclude_bands(scfres::NamedTuple)
n_bands = length(scfres.eigenvalues[1])
exclude_bands = (scfres.n_bands_converge+1):n_bands
exclude_bands
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you need this?

@antoine-levitt
Copy link
Member

Thanks for the pass @mfherbst , I'll take the next one!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants