-
Notifications
You must be signed in to change notification settings - Fork 89
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
Add basic Wannier.jl integration #899
Conversation
Junfeng pushed a 0.3.2 Wannier update, unfortunately this requires Julia 1.8.4 at least. Is it acceptable to update Julia to 1.8, at least for testing? (I have 1.9 on my machine and did not catch this issue...) |
Yes #905. This was in the pipeline for other PRs as well. |
I added hydrogenic projections as an initial guess, because I wanted to try using them. The code was mostly adapted from #854 so I added a co-author note. On the basic example in DFTK, we go from 45 to 17 function calls for the optimization in the Wannierization procedure. |
@mfherbst CI is passing (except for nightly due to a |
Merge conflicts addressed. :) |
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 nice and sorry that it took me so long to reply.
All comments addressed, and I moved the Wannier.jl and Wannier90 integrations to extensions as discussed earlier. We have to wait for another Wannier bump due to the Spglib bump (qiaojunfeng/Wannier.jl#35). |
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.
Small nits, otherwise ok. But definitely not the fastest code ... there are a few type instabilities and some smaller things where performance could be gained, but probably ok for now.
src/external/wannier_shared.jl
Outdated
""" | ||
A p-like initial guess, using the difference of two Gaussians with different centers. | ||
""" | ||
function opposite_gaussians_projection(center1::AbstractVector, center2::AbstractVector) |
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 don't like the idea of directly calling it plike and slike ? I'm not sure myself, but wanted to argue.
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'm not too sure myself. If you already know that you want a plike orbital you probably want to use HydrogenicWannierProjection
anyway (or even the pswfc from the PSP in the future).
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, that's true. Good point. OTOH this makes it questionable why this function even exists in the main code. Might be good to include in the example to show how custom projectors could be made. Maybe it's better put there.
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.
Just removed it. I don't think that encouraging custom projectors is particularly a good idea, in principle it shouldn't be needed and the interface is still experimental. (I was using opposite_gaussians_projection
before the hydrogenic guess was implemented, now it can be useful for comparison against the hydrogenic guess but that doesn't need to be in the mainline code).
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 was using opposite_gaussians_projection before the hydrogenic guess was implemented, now it can be useful for comparison against the hydrogenic guess but that doesn't need to be in the mainline code).
Yes, but it would be good to keep the idea documented in the wannier example. That's what I meant. Sorry if I was not clear.
This PR adds basic
Wannier.jl
integration. The communication happens via the Wannier90 files at the moment; this is not the fastest, but it required the least amount of code. I split the Wannier code acrosswannier_shared.jl
,wannier90.jl
andwannier.jl
, the last 2 requiring the relevant package.I also refactored the initial projection code, to make the choice of the initial projections more flexible. Hopefully that can be extended in the future to project onto atomic orbitals without changing the API again.
Following the recommendation in one of the #854 comments, I updated the example to mention
Wannier.jl
first.