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

effective mass fitting improve #44

Merged
merged 10 commits into from
Jan 14, 2024
Merged

effective mass fitting improve #44

merged 10 commits into from
Jan 14, 2024

Conversation

zhubonan
Copy link
Contributor

Improve the robustness of effective mass fitting algorithm.
CLI and notebook examples are added.

Make the algorithm more robust - now filter by intensity first.
Also added ability to manually pass the extrema points when
computing effective masses.
@kavanase
Copy link
Member

kavanase commented Jan 11, 2024

Thanks for getting to this @zhubonan!

Looks mostly good to me. Some small things:

  • Could we add some docstrings, specifically to get_effective_masses (used in the example) and get_band_extrema, EffectiveMass.
  • The CLI eff mass output ends with:

Unfolded band structure can be ambiguous, please cross-check with the spectral function plot.

This isn't very clear. Can we update this? Cross-check what with the spectral function plot? That the eff mass (or extrema) values make sense from eyeballing?

  • The eff mass example ends with this:

To obtain reliable effective masses, one should generate finely spaced kpoint paths that includes only the vicinity of the CBM/VBM.

This is a bit ominous on its own without any advice to ensure this. Can we add something here about what the user should check? (And I guess if it's not likely converged, then the advice is the user should recalculate the band structure with denser kpoints?)

@kavanase
Copy link
Member

This looks great, thanks for the updates @zhubonan!

@kavanase
Copy link
Member

Is this ready to merge @zhubonan?
I guess it solves #5 so can also close that. Might be worth adding whatever docstrings/notes you wanted to add for #38 now as well before merging? 😃

@zhubonan
Copy link
Contributor Author

Yeah, will see if I can sort out #38 here as well.

@kavanase
Copy link
Member

Great, looks good to me! Let's go ahead and merge this so? 😃

@zhubonan zhubonan merged commit 918a729 into main Jan 14, 2024
13 checks passed
@kavanase kavanase deleted the pr-effective-mass-improve branch January 14, 2024 14:17
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 this pull request may close these issues.

2 participants