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

Maybe plot non-converged bands differently #938

Open
Technici4n opened this issue Dec 27, 2023 · 8 comments · May be fixed by #940
Open

Maybe plot non-converged bands differently #938

Technici4n opened this issue Dec 27, 2023 · 8 comments · May be fixed by #940

Comments

@Technici4n
Copy link
Contributor

Hi, the bandstructure plots also contain bands that are not fully converged (if I understand n_bands_converge in AdaptiveBands correctly). This can be misleading if you expect all plotted bands to be converged.

To give some background:
I encountered the following issue with Wannierization, where the disentanglement step would fail to select the right band close to the Gamma point (see the red line). It turns out that I was only passing the 15 first bands to Wannier, but the expected band is the 16th. I assumed that the plot was only showing the first 15 bands because I set n_bands_converge=15.
image

Would it be a good idea to plot non-converged bands a bit differently? (For example with a dotted line).

@antoine-levitt
Copy link
Member

antoine-levitt commented Dec 27, 2023

Thanks! Wouldn't it just make sense to only plot the converged bands? (we should possibly audit all uses of scfres for this, eg for the DOS plot also). I don't remember why we decided to keep them rather than have a set of extra bands, do you remember @mfherbst ?

(This is graphene, right? This is particularly tricky because it looks like the band of interest is not bound at the gamma point. There are a bunch of parabolic bands that are free electron like in the z direction, and if you increase the size of the cell in the z direction they'll get denser.)

@Technici4n
Copy link
Contributor Author

(Yes this is graphene. It works well with a pz initial guess (added in #899) and with n_bands_converge=16 :))

@mfherbst
Copy link
Member

mfherbst commented Dec 28, 2023

I don't remember why we decided to keep them rather than have a set of extra bands, do you remember @mfherbst ?

I don't know, actually. I guess the rational was to unify the data format during SCF and after to avoid conversion every time you checkpoint (external split format versus internal combined format) or continue an SCF from a returned scfres at a later point.

Indeed we should check we do this consistently. In any case worth adding that the issue of @Technici4n is not the band plotting, since if you go via compute_bands only converged bands are returned (unlike the self_consistent_field). So the real issue is that the wannierisation interface indeed lets you use the non-converged bands (same as the DOS stuff afaik).

@mfherbst mfherbst linked a pull request Dec 28, 2023 that will close this issue
2 tasks
@mfherbst
Copy link
Member

Actually also in many routines (e.g. DOS) it does not matter as the occupation is basically zero.

@Technici4n
Copy link
Contributor Author

the issue of @Technici4n is not the band plotting

I think it is, let me explain. 😄

I was using the default number of bands for the Wannier interface, but I set n_bands_converge too low for graphene.

But I didn't see that n_bands_converge was too low because the plot was plotting more. (see

default_n_bands_bandstructure(n_bands_scf::Int) = ceil(Int, n_bands_scf + 5sqrt(n_bands_scf))
).

In other words I thought I had enough bands because I saw all the bands I needed on the plot.

@mfherbst
Copy link
Member

Ah ok, but that could indeed be solved with a warning in the wannierisation as I suggest in #940.

In general the SCF just computes as many bands as needed to converge the density (and thus obtain a meaningful self-consistent density). Post-processing routines might make different choices for user convenience.

But I agree that the current (master) state is confusing and I can see how you were mislead.

@antoine-levitt
Copy link
Member

In general the SCF just computes as many bands as needed to converge the density (and thus obtain a meaningful self-consistent density). Post-processing routines might make different choices for user convenience.

Hm. Maybe we should rethink that and have eg scfres return views into the full arrays with only the converged ones? But then the scfres would be inconsistent (ie scfres.rho is not the same as compute_density(scfres.psi))... We really should have an scfres struct where we could document this better...

@Technici4n
Copy link
Contributor Author

The warning in the wannierization doesn't hurt but it wouldn't have helped me.

  • I ran an SCF with n_bands_converge=15.
  • The plot showed more bands than 15, including the one I needed for Wannierization (the 16th).
  • I ran Wannierization with the default number of bands (scfres.n_bands_converge which was 15 here).

Wannierization already behaves as expected IMO, the only confusing thing was the plot showing more bands than fully converged in the SCF. (I guess that makes sense because compute_bands can diagonalize the Hamiltonian with more eigenstates.)

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 a pull request may close this issue.

3 participants