-
Notifications
You must be signed in to change notification settings - Fork 97
Add Hubbard corrections and DFT+U #1158
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
Conversation
Technici4n
left a comment
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 haven't looked at the math yet, but have some comments already
test/hamiltonian_consistency.jl
Outdated
| test_consistency_term(ExternalFromFourier(X -> abs(norm(X)))) | ||
| test_consistency_term(LocalNonlinearity(ρ -> ρ^2)) | ||
| test_consistency_term(Hartree()) | ||
| test_consistency_term(Hubbard(DFTK.OrbitalManifold(;species=:Si, label="3S"), 1.0)) |
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.
| test_consistency_term(Hubbard(DFTK.OrbitalManifold(;species=:Si, label="3S"), 1.0)) | |
| test_consistency_term(Hubbard(DFTK.OrbitalManifold(; label="3D"), 1.0)) |
Spacing, and let's use P to test multiple m quantum numbers. And does it work if species is removed as well?
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.
No it doesn't work without species, should it? The intended way should be (in the multiU update that we'll make after this pr is resolved) to add each of the manifolds one by one (so to get a 3d manifold you explicitly ask for all of the single atomic manifolds having 3d orbitals)
- New handling of measure unit for U value; - Hubbard matrix has been renamed everywhere as "nhubbard"; - Function "compute_hubbard_nIJ" has been renamed as "compute_nhubbard"; - New documentation for "compute_nhubbard" function; - New documentation for "Hubbard" struct; - New test on Wigner matrices added in tests/hubbard.jl; - Refactoring inside apply! for HubbardUOperator; - Several lines have been shortened for compactness; - Unutilized functions have been removed from terms/hubbard.jl; - Function reshape_hubbard_proj has been updated.
- New handling of measure unit for U value;
- Hubbard matrix has been renamed everywhere as "nhubbard";
- Function "compute_hubbard_nIJ" has been renamed as "compute_nhubbard";
- New documentation for "compute_nhubbard" function;
- New documentation for "Hubbard" struct;
- New test on Wigner matrices added in tests/hubbard.jl;
- Refactoring inside apply! for HubbardUOperator;
- Several lines have been shortened for compactness;
- Unutilized functions have been removed from terms/hubbard.jl;
- Function reshape_hubbard_proj has been updated.
src/terms/hubbard.jl
Outdated
| U :: Float64 | ||
| end | ||
| function (hubbard::Hubbard)(basis::AbstractBasis) | ||
| isempty(hubbard.U) && return TermNoop() |
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 think it's weird. And it's probably not what you're looking for:
julia> isempty(0)
false
15ea5c0 to
053450f
Compare
- Kwarg use_nlcc=false has been added for all meta_gga tests; - Kwarg spin_polarization=:none added to anyonic test, to avoid failure from @Assert length(basis.kpoints) == 1; - Tolerance for condition number in wigner_d_matrix increased to 100, neq decreased to 2*l+2.
053450f to
d0cf01a
Compare
src/terms/hubbard.jl
Outdated
| natoms = length(manifold_atoms) # Number of atoms of the species in the manifold | ||
| nhubbard = Array{Matrix{Complex{T}}}(undef, nspins, natoms, natoms) | ||
| p_I = [Vector{Matrix{Complex{T}}}(undef, natoms) for ik in 1:length(basis.kpoints)] | ||
| # Very low-level, but works |
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.
Extract l from first label, make sure that all the labels have the same l, and that each block of 2l+1 consecutive labels is the same except for m.
|
@antoine-levitt you said you wanted to have a look, PR is ready! |
|
@antoine-levitt do you still want to have a look or should we go ahead with this PR? |
|
Sorry I didn't see your previous message (got lost in the flood of notifications). I'll take a look! |
antoine-levitt
left a comment
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.
Looks nice! I just have an issue with the manifold selection API. Needs to be clearer that one atomic orbital = one Hubbard term = one l. I would suggest just
struct Hubbard
U
element::Element
atomic_orbital::String
end
struct TermHubbard
U
P
end
and getting rid of the manifold thing entirely. If you want to +U only selected atoms, then just make them different element types.
src/terms/hubbard.jl
Outdated
| stating whether the orbital belongs to the manifold. | ||
| """ | ||
| @kwdef struct OrbitalManifold | ||
| iatom ::Union{Int64, Nothing} = nothing |
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.
Why is this just an int? If I want to put a +U on two atom types, should I have two hubbard terms?
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.
At the moment it is not possible to have more than one +U correction, but I have a different version of the code which allows "multi-U" calculations with different atom types. I was planning to add it as a different PR once this one is closed.
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 think since it very much determines the structure of the code we should do it now
src/terms/hubbard.jl
Outdated
| where n or ibnd is the band index, ``weights[ik ibnd] = kweights[ik] * occupation[ik, ibnd]`` | ||
| and ``Pᵢₘ₁`` is the pseudoatomic orbital projector for atom i and orbital m₁ | ||
| (usually just the magnetic quantum number, since l is usually fixed). |
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 not a "usually" it's enforced in the code, right? So it's actually enforced that each Hubbard term only deals with one type of atomic orbital. This needs to be enforced by the design.
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.
In the code this is enforced, yes, because the symmetrization of the nhubbard is l-dipendent.
mfherbst
left a comment
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 @fsicignano for updating this so quickly. I think it indeed makes for a cleaner code.
|
There is one more thing: currently the example takes a few minutes to run, maybe its discretization should be made coarser. |
Yes, I agree. If we can, we should. But with magnetism that's sometimes not possible. |
|
I think we are good now! |
|
@antoine-levitt now is the time if you want to have another look :) |
|
LGTM, thanks for doing this! |
New Hubbard U term implementation.
Key features: - New hubbard.jl program
- New operator added in operators.jl
- OrbitalManifold struct has been moved to hubbard.jl
- self_consistent_field.jl has been updated to store the Hubbard n matrix and the occupation
- Hubbard term has been added to the tests in hamiltonian_consistency
- The pseudopotential used in hamiltonian_consistency has been changed with one having atomic orbitals