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

Scissor operator to conduction band #83

Merged
merged 1 commit into from
Feb 15, 2023
Merged

Scissor operator to conduction band #83

merged 1 commit into from
Feb 15, 2023

Conversation

mrm24
Copy link
Contributor

@mrm24 mrm24 commented Feb 13, 2023

Added scissor operator to conduction band, for cases in which such a correction might be essential, like Ge. Tested against test case (Si) but with n-doping (log files attached). The used scissor operator for this case was that necessary for gap to match that of Varshni's model [Y. Varshni, Physica 34, 149 (1967)] for Si at 300 K.
noscissor.log
scissor.log

@nakib
Copy link
Owner

nakib commented Feb 13, 2023

Dear Martí,

Thanks for creating this PR. I will go over it in the coming days and get back to you if I have questions.

Best regards,
Nakib

@nakib nakib self-assigned this Feb 13, 2023
@nakib nakib added the enhancement New feature or request label Feb 13, 2023
@nakib
Copy link
Owner

nakib commented Feb 14, 2023

Dear Martí,

I have begun reviewing the PR. One general comment: in your current design, one has to keep track of where el_wann_epw is called, and then remember to apply the scissor shift. For example, the member procedure plot_along_path of the epw_wannier type also calls el_wann_epw but the scissor shift there is missing. As such, I think it might be more elegant to apply the scissor shift where the energy is calculated, i.e. inside el_wann_epw. Let me know what you think of this.

A second point. I have a bunch of new changes that I will soon merge from the branch green. Just a heads-up. ;)

Best,
Nakib

@mrm24
Copy link
Contributor Author

mrm24 commented Feb 14, 2023

Dear Nakib,

You're right, I've fully forgotten about band plotting; indeed, your solution looks cleaner.
From an input point of view, I see it more logical to add the scissor in the electron type, thus it should be better to pass the necessary variables to the el_wann_epw function, what do you think? If you are ok with that solution, I can rewrite it to do so in that way.

Moreover, one thing that I forgot about is that it should be forbidden to use the scissor operator if the system is defined as metallic.

Best regards,

Martí

PS: Shall I do changes over Green branch to prevent merging conflicts then? Anyway, I'll take a look at the additions :)

@nakib
Copy link
Owner

nakib commented Feb 14, 2023

Dear Martí,

I agree that it should be the electron data type constructor that should set the scissor shift parameter. So I am fine with what you have done at the top of the electron module. I also agree that the scissor shift amount should be sent to el_wann_epw. One option could be to pass the scissor shift amount as an optional argument to el_wann_epw. If it is present, only then the scissor operation is applied to the output energies.

Regarding your comment about metallic systems, I agree. You can simple enforce the scissor amount to be zero if metallic is true when you are reading the inputs, or, alternatively, call exit_with_message.

Right now develop is up-to-date with green. It would be great if you updated your fork.

Many thanks for contributing!

Best regards,
Nakib

@mrm24
Copy link
Contributor Author

mrm24 commented Feb 14, 2023

Dear Nakib,

I've rewritten it on top of develop branch with all the modifications we've discussed. For such changes, I've supposed that wann%numwannbands must be equal to el%numbands. Is this last assumption always true ?

Martí

@mrm24 mrm24 reopened this Feb 14, 2023
@nakib
Copy link
Owner

nakib commented Feb 14, 2023

Yes, el%numbands should always be the same as wann%numwannbands.

Could you please share your outputs for the silicon case with and without the scissor shift? In particular, if you set up a calculation such that only the conduction bands are transport active (you can do this by setting indlowband to the index of the lowest conduction band) in both the case, the transport coefficients should be identical.

p.s. I have set up a regression test, but the necessary input files are too large for sharing on github; I'm working on a solution for this.

Best,
Nakib

@mrm24
Copy link
Contributor Author

mrm24 commented Feb 14, 2023

Dear Nakib,

I've conducted the test as implemented in my version of the repository with and without the scissor applied (see noscissor.log and scissor.log, for the one without and with the scissor operator applied, respectively), and as you can see the transport coefficients are different. I've tracked back the difference and is due to the delta_fn_triang function which provides different deltas for the scissored mesh. Indeed, by introducing an unshifted mesh in the fill_triangles within calculate_electrons, plus the removal of the scissor in the first variable of the delta_fn_triang function; the result for transport properties becomes identical (see scissor-modif.log) even with the rest of the variables scissored.

Shall I regularize the energies to build and compute delta functions by removing the chemical potential, so that, the only transitions in which delta would become scissor dependent would be those involving valence and conduction states?

Best regards,

Martí

@mrm24
Copy link
Contributor Author

mrm24 commented Feb 14, 2023

Forgot about the regularization idea, it makes no sense.

@nakib
Copy link
Owner

nakib commented Feb 14, 2023

Hi Martí,

Let me think about this for a bit. I will get back to you soon.

Best
Nakib

@nakib
Copy link
Owner

nakib commented Feb 15, 2023

Inspecting the files noscissor.log and scissor.log, things look ok to me. But I am surprised that delta_fn_triang causes a difference in the delta weights since all the expressions there only use differences of energies. Your code looks fine to me, and my best guess is that the issue is numerical in nature because the difference caused by these different triangle weights is small. If there were catastrophic errors in the energy shifting procedure, the final results would be quite different since your shift in the test case is quite large (0.5 eV). I'm curious if you have checked whether the difference between the the unshifted and shifted cases increases/decreases if you make the shift amount huge (100 eV!)/small (0.01 eV).

Best,
Nakib

@mrm24
Copy link
Contributor Author

mrm24 commented Feb 15, 2023

Dear Nakib,

The difference (error) grows with the scissor amount but it seems to somehow saturate. See the attached files:
small.log (0.01 eV), big.log (100.0 eV), and giant.log (1000.0 eV). I guess, this further supports numerical issues due to numerical finite precision.

Additionally, as reference of the values expected to take this scissor operator in common semiconductors, take the case of Si (Ge), which requires around 0.6 (0.8) eV scissor to match the experimental gap. This last is especially crucial for Ge, which has no band-gap within DFT-LDA, and thus will have nonphysical transitions allowed without such a correction.

Another option besides the scissor operator (maybe a future enhancement), and to be fair the physically correct one (as you remove empirical stuff) would be to allow elphbolt to read GW corrected energy files, as EPW does via the eig_read=.true. card.

Best regards,

Martí

@nakib
Copy link
Owner

nakib commented Feb 15, 2023

Thanks, Martí. The numerical issue is a bit worrisome, and I will take a closer look at how the triangular weights are calculated. Perhaps one can use some tricks there to remove this issue coming from scissor shifts.

I noticed that you have left it up to the user to scissor shift their reference energy and chemical potential directly at the level of the input. I agree with this choice. I will make a note of this in the documentation.

Regarding GW corrections, I agree. In fact, GW will correct the band mass as well, which is critical for mobility calculations. I hope to have updates regarding this soon.

I am currently setting up github actions for this repo and will accept this merge request right after.

Thanks again for your contribution!

@nakib nakib merged commit 381aa91 into nakib:develop Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants