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

[Core] Modify coordinate_transformation_utilities.h to allow transformation back to global coordinates #12327

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gzjing
Copy link
Contributor

@gzjing gzjing commented Apr 27, 2024

The coordinate_transformation_utilities.h is modified slightly to allow rotation of the contributions back to the global frame of reference. This change is necessary for the refactoring of particle-based slip for the MPMApplication (#12018), where it was decided to rotate the contributions of particle-based conditions back to the global frame of reference after correcting for SLIP to avoid complications with mis-aligned frames of reference.

The changes made are as follows:

  • Use template boolean TToGlobalCoord (default false) in RotateAux and RotateAuxPure, transposing the rotation matrix when this is set to true to allow rotation back to the global coordinate system
  • Extracted RHS version of Rotate to RotateRHSAux to allow a similar process to be done without affecting the existing interface

@gzjing gzjing requested a review from a team as a code owner April 27, 2024 22:21
@gzjing gzjing changed the title Modify coordinate_transformation_utilities.h to allow transformation back to global coordinates [Core] Modify coordinate_transformation_utilities.h to allow transformation back to global coordinates May 3, 2024
@VeronikaSinger
Copy link
Contributor

Hi @KratosMultiphysics/technical-committee
do you agree with these code changes? We require the refactoring of the rotation utility to use it also for the rotation back to the global frame of reference.

@VeronikaSinger
Copy link
Contributor

@KratosMultiphysics/technical-committee any comments on this small change?


}

RotateRHSAux(rLocalVector, rGeometry);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
RotateRHSAux(rLocalVector, rGeometry);
RotateRHSAux(rLocalVector, rGeometry);

Please follow the indentation rules.

@@ -766,6 +789,8 @@ class CoordinateTransformationUtils {
DenseVector<bool> NeedRotation( NumBlocks, false);

std::vector< BoundedMatrix<double,TBlockSize,TBlockSize> > rRot(NumBlocks);
BoundedMatrix<double,TBlockSize,TBlockSize> tmp;
Copy link
Member

Choose a reason for hiding this comment

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

Indentation.

@@ -846,6 +877,8 @@ class CoordinateTransformationUtils {
DenseVector<bool> NeedRotation( NumBlocks, false);

std::vector< BoundedMatrix<double,TDim,TDim> > rRot(NumBlocks);
BoundedMatrix<double,TDim,TDim> tmp;
Copy link
Member

Choose a reason for hiding this comment

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

Indentation.

Copy link
Member

@rubenzorrilla rubenzorrilla left a comment

Choose a reason for hiding this comment

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

The usage is IMO a bit cryptic. Instead of hacking the current implementation by transposing the matrix and adding a template argument, which value is defaulted, I'd have added a new method "BackToOriginalSystem"). The implementation can be done the way you did but making the RotateRHSAux private rather than protected.

Also note that GlobalCoord commonly refers to isoparametric transformation (see our geometries for instance). In this case what you're actually doing is to rotate the system of coordinates. Hence, the naming is also misleading IMO.

@gzjing
Copy link
Contributor Author

gzjing commented Jul 2, 2024

@rubenzorrilla I agree the usage is a little cryptic. The idea was to have something that can perform the inverse rotation to recover the original coordinate system without affecting other existing code, and doing it this way ensures that the rotation/inverse rotations are always performed the same way while avoiding code duplication.

Agreed on the misleading naming given the normal usage of GlobalCoord -- sorry!

I suppose I could indeed define new methods RevertRotate[RHS] alongside Rotate[RHS], both of which then delegates the call to a common private function. The private function can similarly be templated or have another boolean argument, say inverseRotation. Would this be better?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants