-
Notifications
You must be signed in to change notification settings - Fork 882
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
Convert all FHI-aims stresses to be 3x3 instead of Voigt notation #3476
Conversation
Previously used voigt
pymatgen/io/aims/parsers.py
Outdated
@@ -633,7 +639,7 @@ def forces(self) -> list[Vector3D] | None: | |||
) | |||
|
|||
@property | |||
def stresses(self) -> list[Matrix3D] | None: | |||
def stresses(self) -> Sequence[Matrix3D] | None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better not use sequence here. best to be as generic as possible in arguments and as specific as possible in return values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fixed now, but I am not sure what caused the CI errors for VASP
Use np array instead of Sequence Signed-off-by: Thomas Purcell <tpurcell90@users.noreply.github.com>
pymatgen/io/aims/parsers.py
Outdated
def voigt_to_full_stress_conv(voigt_stress: Sequence[float]) -> Matrix3D: | ||
"""Transform voigt vector to a full 3x3 matrix.""" | ||
xx, yy, zz, yz, xz, xy = np.array(voigt_stress).flatten() | ||
return np.array([[xx, xy, xz], [xy, yy, yz], [xz, yz, zz]]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we use ase.stress.voigt_6_to_full_3x3_stress
instead of defining our own?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can I was not sure the recommendation was for the use of ASE functions in non-ASE parts since it is an optional dependency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You were already importing from ase.stress
but I just realized that was in a test file. You're right, in a non-test file, we have to wrap in try: import ase except ImportError: ...
.
Better still though, we already have the Tensor
class for this. It has a from_voigt
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I converted this to use the Tensor object instead (sorry for the delay I got my new personal computer so can now actually work again)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! 👍
This should now be more consistent
Previously used voigt
Summary
Major changes:
Checklist
ruff
.mypy
.duecredit
@due.dcite
decorators to reference relevant papers by DOI (example)