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

Convert all FHI-aims stresses to be 3x3 instead of Voigt notation #3476

Merged
merged 4 commits into from
Nov 21, 2023

Conversation

tpurcell90
Copy link
Contributor

Previously used voigt

Summary

Major changes:

  • fix 1: all stresses are 3x3 now

Checklist

  • Google format doc strings added. Check with ruff.
  • Type annotations included. Check with mypy.
  • Tests added for new features/fixes.
  • If applicable, new classes/functions/modules have duecredit @due.dcite decorators to reference relevant papers by DOI (example)

@@ -633,7 +639,7 @@ def forces(self) -> list[Vector3D] | None:
)

@property
def stresses(self) -> list[Matrix3D] | None:
def stresses(self) -> Sequence[Matrix3D] | None:
Copy link
Member

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.

Copy link
Contributor Author

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>
Comment on lines 61 to 64
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]])
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Contributor Author

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)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! 👍

@janosh janosh changed the title Convert all FHI-aims stresses to be 3x3 Convert all FHI-aims stresses to be 3x3 instead of Voigt notation Nov 21, 2023
@janosh janosh added io Input/output functionality fix Bug fix PRs labels Nov 21, 2023
@janosh janosh merged commit ec750ca into materialsproject:master Nov 21, 2023
20 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fix PRs io Input/output functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants