Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

SCDM method in Wannier90 #167

Merged
merged 9 commits into from
Feb 21, 2018
Merged

Conversation

VVitale
Copy link
Collaborator

@VVitale VVitale commented Feb 14, 2018

Major commit for the SCDM method. Files modified are: parameters.F90 kmesh.F90 and pw2wannier90.F90, the latter being the one with the actual code for the projections.
I have also added the necessary info in the documentation (user guide) on how to use the new projection method.

Copy link
Member

@giovannipizzi giovannipizzi left a comment

Choose a reason for hiding this comment

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

As an additional comment - no need to recompile the docs, as the repository becomes very big - we can recompile it when we will do a release.

@@ -611,7 +614,8 @@ \subsection{Shells}
\section{Projection}

The projections block defines a set of localised functions used to
generate an initial guess for the unitary transformations.
generate an initial guess for the unitary transformations. The projection block can be specified
in conjunction with {\tt scdm\_proj=true} (see belove). This is only used to read the centers of the projections, which in some cases could help the optimisation if {\tt guiding\_centres=true} is added to the input file.
Copy link
Member

Choose a reason for hiding this comment

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

belove->below

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@@ -736,6 +740,31 @@ \section{Job Control}

The default value of this parameter is $\verb#false#$.

\subsection[scdm\_proj]{\tt logical :: scdm\_proj}
If {\tt scdm\_proj=true} then the $A_{mn}^{(\mathbf{k})}$ matrices are generated with the SCDM-k method of Ref.~\cite{LinLin-ArXiv2017}. In this case, one also needs to specify the other three {\tt scdm} keywords. One then needs to run {\tt wannier90.x -pp seedname} to generate the {\tt seedname.nnkp} file, to be used by a first-principle code (at the moment only interface available is with the QuantumEspresso code). It is not possible to set {\tt scdm\_proj=true} and simultaneously specify a projection block.
Copy link
Member

Choose a reason for hiding this comment

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

Are mu and sigma really always needed to be specified, also in the isolated case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No they don't you are right. I have modified the text according to the logic.

if(found .and. scdm_proj .and. spinors) &
call io_error('Error: SCDM method is not compatible with spinors yet.')
if(found .and. scdm_proj .and. guiding_centres) &
call io_error('Error: guiding_centre is not compatible with the SCDM method yet.')
Copy link
Member

Choose a reason for hiding this comment

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

guiding_centres

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

elseif(ctmp=='erfc') then
scdm_entanglement = 1
elseif(ctmp=='gaussian') then
scdm_entanglement = 1
Copy link
Member

Choose a reason for hiding this comment

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

should be 2!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops! This one is more serious.

\subsection[scdm\_sigma]{\tt real(kind=dp) :: scdm\_sigma}
The value of the $\sigma$ parameter in the formulas for the occupation numbers matrix. It defines the spread of the occupation numbers matrix around $\mu$, and as such it cannot be negative. It must be given in units of eV.

The default value is {\tt 0.0 eV}
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if setting the default to zero is a good idea, will give rise to division by zero. Better to give e.g. a default of 2eV, or 4eV? Or leave to zero, but then in parameters.F90 check if it is zero and complain that it has to be specified or > 0.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now sigma is 1.0 eV by default. Then in parameters.F90 it checks if sigma is specified and it returns an error if it is not positive.

\subsection[scdm\_mu]{\tt real(kind=dp) :: scdm\_mu}
The value of the $\mu$ parameter in the formulas above. It defines the characteristic energy for the occupation numbers matrix, in units of eV. If {\tt scdm\_entanglement=1}, it gives the mean value of the complementary error function. If {\tt scdm\_entanglement=2}, it gives the mean value of the gaussian instead.

The default value is {\tt fermi\_energy eV}
Copy link
Member

Choose a reason for hiding this comment

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

However, from the logic in parameters.F90, if the fermi_energy is not specified, the default remains at zero eV... maybe better to simplify the logic, and leave this value by default at 0 eV? If the user specifies by hand the fermi_energy flag, he can also set it twice here. Also, in our experience mu=fermi is not always the optimal choice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now mu is 0.0 eV by default.


\subsection[scdm\_entanglement]{\tt integer :: scdm\_entanglement}
Select the functional form for the occupation number matrix $f(\epsilon_{n\mathbf{k}})$ for the SCDM-k method.
Only three integer values are allowed:
Copy link
Member

Choose a reason for hiding this comment

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

This is not currently true, in the input the user specifies three strings - internally they are converted to an integer

READ (iun_nnkp,*) scdm_mu, scdm_sigma
ENDIF
ELSE
WRITE(stdout,'(//," ****** begin WARNING ****** ",/)')
Copy link
Member

Choose a reason for hiding this comment

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

Do we really want to issue a Warning? If the user does not want to use SCDM, then he does not need to upgrade to a recent version. If he wants to, then he will have to have already upgraded.
This warning might "scare" users but I don't think it is needed (most probably they will read this and don't know what SCDM is).

@giovannipizzi giovannipizzi self-assigned this Feb 15, 2018
@hjunlee
Copy link
Contributor

hjunlee commented Feb 16, 2018

Dear developers:

Although I tried to do the similar things in both USPP in QE and PAW in VASP, I am still wondering about the extension of SCDM to USPP and PAW where the norm of PW parts only is not equal to 1.

In the pw2wannier90, you represent only (soft) PW parts of wave functions on the real grid and you enforce normalization of psic_all by dividing norm_psi. As a consequence, since in general norm_psi is different between different band indices and thus, psic_all with different band indices are scaled by the different factors.

My question is whether or not piv is affected by these things.

I know well that even in NCPP case, psi is not meaningful near the nucleus of the atoms and I also guess that in PAW and USPP cases, we don't need to construct psi exactly considering augmentation parts. But, I am still wondering whether the enforcement of normalization in USPP and PAW cases works in all (or most) cases.

Sincerely,

Hyungjun Lee
EPFL SB Institute of Physics

@giovannipizzi
Copy link
Member

Hi, my guess is that the current implementation is (very often) ok (at least for the cases that we have tested). BTW, I think this is the same the authors of the original method did in their Matlab implementation. We're doing a more thorough validation study with more systems in the meantime.

@VVitale There's a typo in the code that prevents compilation I think: https://travis-ci.org/wannier-developers/wannier90/jobs/343964349#L600 could you fix it?

@codecov
Copy link

codecov bot commented Feb 20, 2018

Codecov Report

Merging #167 into develop will decrease coverage by <.01%.
The diff coverage is 58.33%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #167      +/-   ##
===========================================
- Coverage    57.42%   57.41%   -0.01%     
===========================================
  Files           27       27              
  Lines        15538    15573      +35     
===========================================
+ Hits          8922     8942      +20     
- Misses        6616     6631      +15
Impacted Files Coverage Δ
src/kmesh.F90 60.19% <50%> (-0.11%) ⬇️
src/parameters.F90 77.55% <60.71%> (-0.16%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6507313...ad15a23. Read the comment docs.

@giovannipizzi giovannipizzi merged commit 85f143f into wannier-developers:develop Feb 21, 2018
@hjunlee
Copy link
Contributor

hjunlee commented Feb 21, 2018

It seems to me that there is a very minor bug in pw2wannier90:
In a serial case, the phase array has a wrong dimension:
phase(iw,iw)

Sincerely,

Hyungjun Lee
EPFL SB IoP

manxkim pushed a commit to manxkim/wannier90 that referenced this pull request Jan 10, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants