This class has grown too convoluted and with contradictory standards in indexing. It should be streamlined, better commented and made more consistent.
Check Riccardo's comments in issue yambo-code/yambopy-devel#43:
Hi, I add a deeper look into this, and I was able to reproduce the problem.
I have few comments on the routines related to this plot in the YamboQPDB class.
In this file, the band index counting start from 1 instead of 0 and this is creating quite few inconsistencies. First, we should set a convention if we want in the tutorial file n_top_vb to be 4 (case a) or 3 (case b). If I understood correctly, in this example we have 4 valence bands and 4 conduction bands. Therefore, it makes sense to set n_top_vb=4. Note that self.min_band=1 and self.min_band=8 (instead of 0 and 7 as usual in Python counting).
get_direct_gaps : this function should indeed give the direct gaps, and it does not currently work for case b) due to this counting issue. A solution could be to replace the line shifted_valence = valence-self.min_band+1 with shifted_valence = valence-self.min_band+2.
plot_scissor_ax: this function has been changed. Before we were plotting the points as given from the linear relationship. y = mx +q. Now instead, we are plotting the "qp" energies against the dft ones. However, I don't think it makes sense to plot the difference between the 'qp' value and the '0' one, as in ax.plot(v0, vqp-v0). A possible solution would be to plot ax.scatter(ce0,cqp-cintercept) or ax.scatter(ce0,cqp). Regarding case a) and b), in this function we have to be particularly careful in the calls for get_filtered_qps. Depending on the convention that we choose, the calls to get_filtered_qps would have to change accordingly.
Please, give me your comments and thoughts so that we can proceed accordingly to the set convention.
Anyway, I forked my branch and can fix the error, but it is not clear to me how we should handle bug-fixes in the GPL release. Should I clone it and create merge requests?
Originally posted by @rreho in https://github.com/yambo-code/yambopy-devel/issues/43#issuecomment-1628494250
This class has grown too convoluted and with contradictory standards in indexing. It should be streamlined, better commented and made more consistent.
Check Riccardo's comments in issue yambo-code/yambopy-devel#43:
Hi, I add a deeper look into this, and I was able to reproduce the problem.
I have few comments on the routines related to this plot in the YamboQPDB class.
In this file, the band index counting start from 1 instead of 0 and this is creating quite few inconsistencies. First, we should set a convention if we want in the tutorial file
n_top_vbto be 4 (case a) or 3 (case b). If I understood correctly, in this example we have 4 valence bands and 4 conduction bands. Therefore, it makes sense to setn_top_vb=4. Note thatself.min_band=1andself.min_band=8(instead of 0 and 7 as usual in Python counting).get_direct_gaps: this function should indeed give the direct gaps, and it does not currently work for case b) due to this counting issue. A solution could be to replace the lineshifted_valence = valence-self.min_band+1withshifted_valence = valence-self.min_band+2.plot_scissor_ax: this function has been changed. Before we were plotting the points as given from the linear relationship.y = mx +q. Now instead, we are plotting the "qp" energies against the dft ones. However, I don't think it makes sense to plot the difference between the 'qp' value and the '0' one, as inax.plot(v0, vqp-v0). A possible solution would be to plotax.scatter(ce0,cqp-cintercept)orax.scatter(ce0,cqp). Regarding case a) and b), in this function we have to be particularly careful in the calls forget_filtered_qps. Depending on the convention that we choose, the calls toget_filtered_qpswould have to change accordingly.Please, give me your comments and thoughts so that we can proceed accordingly to the set convention.
Anyway, I forked my branch and can fix the error, but it is not clear to me how we should handle bug-fixes in the GPL release. Should I clone it and create merge requests?
Originally posted by @rreho in https://github.com/yambo-code/yambopy-devel/issues/43#issuecomment-1628494250