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

Improve types for electronic_structure.{core/dos} #3880

Merged

Conversation

DanielYang59
Copy link
Contributor

@DanielYang59 DanielYang59 commented Jun 16, 2024

Summary

  • Improve types and docstring cleanups for (follow up of Improve types for electronic_structure.{bandstructure/cohp} #3873):
    • electronic_structure.core
    • electronic_structure.dos
  • Nest private classmethod (which should not be classmethod but staticmethod) get_transformation_matrix inside get_moment method 2843854, rationale:
    • It's used by get_moment alone
    • No unit test added for get_transformation_matrix (usually I would expose such internal function as private method if I need to unit test it)

@DanielYang59 DanielYang59 marked this pull request as ready for review August 3, 2024 15:44
@DanielYang59
Copy link
Contributor Author

@janosh Apologize for trying to mess up your weekend, just ping you to get this tracked as well and no rush at all. Thanks!

Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff! Thanks

@janosh janosh merged commit c185bd4 into materialsproject:master Aug 3, 2024
33 checks passed
@DanielYang59 DanielYang59 deleted the type-elec-struct-core-dos branch August 3, 2024 15:57
@DanielYang59
Copy link
Contributor Author

Thanks! Have a nice weekend ahead!

@janosh janosh added the types Type all the things label Aug 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
types Type all the things
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants