JuliaMolSim / DFTK.jl

Density-functional toolkit
https://docs.dftk.org
MIT License
443 stars 89 forks source link

Maybe plot non-converged bands differently #938

Open Technici4n opened 10 months ago

Technici4n commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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

mfherbst commented 10 months ago

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 commented 10 months ago

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

Technici4n commented 10 months ago

the issue of @Technici4n is not the band plotting

I think it is, let me explain. :smile:

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 https://github.com/JuliaMolSim/DFTK.jl/blob/e5fa7b69c236a6fa7543083da2161bd5df86e000/src/postprocess/band_structure.jl#L265).

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

mfherbst commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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

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.)