[GeoMechanicsApplication] Added a helper function that converts an angle in degrees to one in radians #11112

avdg81 · 2023-05-11T12:47:11Z

📝 Description
Added a helper function that converts an angle in degrees to one in radians. With this function in place, the hard-coded conversions have been replaced by calls to this function.

🆕 Changelog

Introduced nonmember function DegreesToRadians.
Replaced the hard-coded angle conversions in the GeoMechanicsApplication by calling this new function.

philbucher

would be cool to add this to the math_utils in the Core

philbucher · 2023-05-11T15:14:35Z

applications/GeoMechanicsApplication/custom_utilities/math_utilities.cpp

+
+double DegreesToRadians(double AngleInDegrees)
+{
+    return (AngleInDegrees * M_PI) / 180.0;


Suggested change

return (AngleInDegrees * M_PI) / 180.0;

return (AngleInDegrees * Globals::Pi) / 180.0;

AlejandroCornejo · 2023-05-11T15:27:39Z

applications/GeoMechanicsApplication/custom_utilities/math_utilities.cpp

+namespace Kratos
+{
+
+double DegreesToRadians(double AngleInDegrees)


Suggested change

double DegreesToRadians(double AngleInDegrees)

double DegreesToRadians(const double AngleInDegrees)

I'm not sure whether this actually improves the code. I have checked the C++ Core Guidelines and there they explicitly mention this situation as an exception to the rule that objects should be immutable whenever possible. From section Con 1: By default, make objects immutable it says (under the heading Exception):

Function parameters passed by value are rarely mutated, but also rarely declared const. To avoid confusion and lots of false positives, don’t enforce this rule for function parameters.

Also the accompanying example illustrates this:

void g(const int i) { ... } // pedantic

Since I don't see a good reason to deviate from that recommendation here, I'd like to keep it the way it is. (Unless you have a convincing reason why we should make it immutable here.) Thanks.

rubenzorrilla · 2023-05-11T15:46:07Z

applications/GeoMechanicsApplication/custom_utilities/math_utilities.hpp

@@ -1236,5 +1236,8 @@ class StructuralMechanicsMathUtilities

 private:
 };// class StructuralMechanicsMathUtilities
+
+KRATOS_API(GEO_MECHANICS_APPLICATION) double DegreesToRadians(double AngleInDegrees);


You're declaring the function at the global Kratos namespace. Though this is technically possible, in Kratos we always prefer the static but inside a class approach. As suggested by @philbucher, I'd place this within the Kratos core math utils.

Thanks, I will move the function to the Kratos core math utils as suggested by @philbucher and you. However, what is the rationale behind using a static member function rather than a nonmember function? If you're trying to limit its scope (which is a fair point), I feel that using a nested namespace is a simpler and cleaner way to achieve just that.
I will follow your suggestion anyway, but I'm just asking you to think about this one and to consider whether we may want to change this policy. Thanks.

Yes, my point is to basically limit its scope. About how to achieve it, i.e. the class vs namespace, this is a long term discussion in Kratos. At some point we tried to provide some guidelines & conclusions in #7434.

avdg81 · 2023-05-12T13:06:01Z

would be cool to add this to the math_utils in the Core

I have looked into that, but it's not as straightforward as it may seem at first sight. Let me explain. MathUtils is actually a template class, which means it needs a template argument (TDataType) in order to be instantiated. Surely we wouldn't want the return type to be of this type, since undesired things may happen. For instance, consider TDataType to be set to int, it would simply truncate the returned value (which almost always will be a floating point number). That's probably not what you want. So a fixed return type of double seems more logical. You might argue that the input parameter could be of type TDataType, but in the end it needs to be converted to a double anyway. So, if we would express that explicitly (which is how my current function signature looks like), then the template parameter is not used in any way. But if you want to call the function, you'd still have to supply a template argument, which opens the door to silly code constructs like MathUtils<std::string>::DegreesToRadians(90);.

The whole problem here stems from the fact that the template parameter is actually redundant for this particular case. To me, whichever way we go here, it will never be really elegant in my opinion (please correct me if I have overlooked a good option). Therefore, I'm inclined to keep it the way it is now and leave it up to you to move it to a more general location (say somewhere in Kratos core) in a way that you feel best fits the way Kratos does things. Any feedback is greatly appreciated. Thanks.

philbucher · 2023-05-12T19:02:41Z

sounds reasonable

@rubenzorrilla @loumalouomega maybe it is time to drop the template arg for the math utils? I don't think any type other than double is used

loumalouomega · 2023-05-12T19:13:29Z

sounds reasonable

@rubenzorrilla @loumalouomega maybe it is time to drop the template arg for the math utils? I don't think any type other than double is used

I started to remove it long time ago in a branch, I can retake it

rubenzorrilla · 2023-05-15T06:18:34Z

sounds reasonable
@rubenzorrilla @loumalouomega maybe it is time to drop the template arg for the math utils? I don't think any type other than double is used

I started to remove it long time ago in a branch, I can retake it

sounds reasonable

@rubenzorrilla @loumalouomega maybe it is time to drop the template arg for the math utils? I don't think any type other than double is used

I was about to suggest this.

avdg81 · 2023-05-15T07:33:54Z

sounds reasonable
@rubenzorrilla @loumalouomega maybe it is time to drop the template arg for the math utils? I don't think any type other than double is used

I started to remove it long time ago in a branch, I can retake it

Also sounds good to me. If I can contribute in any way to get this change in, I'm more than happy to help. Thanks.

loumalouomega · 2023-05-15T11:18:11Z

sounds reasonable
@rubenzorrilla @loumalouomega maybe it is time to drop the template arg for the math utils? I don't think any type other than double is used

I started to remove it long time ago in a branch, I can retake it

Also sounds good to me. If I can contribute in any way to get this change in, I'm more than happy to help. Thanks.

It is already done, as I said, I had the changes in a local branch and I only needed to do some clean up and push.

philbucher

thanks!

applications/GeoMechanicsApplication/custom_elements/steady_state_Pw_piping_element.cpp

Other changes include: - Replaced `M_PI` by `Globals::Pi`. - Use the `std` variant of functions `sin` and `cos`.

philbucher

Nice!

avdg81 added the GeoMechanics Issues related to the GeoMechanicsApplication label May 11, 2023

avdg81 requested review from mcgicjn2 and huhaas May 11, 2023 12:47

avdg81 linked an issue May 11, 2023 that may be closed by this pull request

[GeoMechanicsApplication] Add utility function for Radians → Degrees and Degrees → Radians conversions. #10946

Closed

philbucher reviewed May 11, 2023

View reviewed changes

AlejandroCornejo reviewed May 11, 2023

View reviewed changes

rubenzorrilla reviewed May 11, 2023

View reviewed changes

loumalouomega mentioned this pull request May 15, 2023

[Core] Transition to remove template argument MathUtils #11130

Merged

avdg81 force-pushed the geo/10946-deg-to-rad-conversion branch from 291a7a3 to 2a69cd4 Compare May 26, 2023 15:43

avdg81 requested a review from a team as a code owner May 26, 2023 15:43

philbucher reviewed May 26, 2023

View reviewed changes

applications/GeoMechanicsApplication/custom_elements/steady_state_Pw_piping_element.cpp Outdated Show resolved Hide resolved

philbucher previously approved these changes May 26, 2023

View reviewed changes

Added a helper function for converting angles from degrees to radians

d79d904

Other changes include: - Replaced `M_PI` by `Globals::Pi`. - Use the `std` variant of functions `sin` and `cos`.

avdg81 dismissed philbucher’s stale review via d79d904 May 30, 2023 07:49

avdg81 force-pushed the geo/10946-deg-to-rad-conversion branch from 2a69cd4 to d79d904 Compare May 30, 2023 07:49

avdg81 requested a review from philbucher May 30, 2023 07:50

philbucher approved these changes May 30, 2023

View reviewed changes

huhaas approved these changes May 30, 2023

View reviewed changes

avdg81 removed the request for review from mcgicjn2 May 30, 2023 09:36

roigcarlo approved these changes May 30, 2023

View reviewed changes

huhaas merged commit 3bc6696 into master May 30, 2023

huhaas deleted the geo/10946-deg-to-rad-conversion branch May 30, 2023 12:32

avdg81 mentioned this pull request May 31, 2023

[MultilevelMonteCarloApplication] Unexpected failure of test_estimation_random #11212

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GeoMechanicsApplication] Added a helper function that converts an angle in degrees to one in radians #11112

[GeoMechanicsApplication] Added a helper function that converts an angle in degrees to one in radians #11112

avdg81 commented May 11, 2023

philbucher left a comment

philbucher May 11, 2023

AlejandroCornejo May 11, 2023

avdg81 May 12, 2023

rubenzorrilla May 11, 2023

avdg81 May 12, 2023

rubenzorrilla May 12, 2023

avdg81 commented May 12, 2023 •

edited

Loading

philbucher commented May 12, 2023

loumalouomega commented May 12, 2023

rubenzorrilla commented May 15, 2023

avdg81 commented May 15, 2023

loumalouomega commented May 15, 2023

philbucher left a comment

philbucher left a comment

	return (AngleInDegrees * M_PI) / 180.0;
	return (AngleInDegrees * Globals::Pi) / 180.0;

	double DegreesToRadians(double AngleInDegrees)
	double DegreesToRadians(const double AngleInDegrees)

[GeoMechanicsApplication] Added a helper function that converts an angle in degrees to one in radians #11112

[GeoMechanicsApplication] Added a helper function that converts an angle in degrees to one in radians #11112

Conversation

avdg81 commented May 11, 2023

philbucher left a comment

Choose a reason for hiding this comment

philbucher May 11, 2023

Choose a reason for hiding this comment

AlejandroCornejo May 11, 2023

Choose a reason for hiding this comment

avdg81 May 12, 2023

Choose a reason for hiding this comment

rubenzorrilla May 11, 2023

Choose a reason for hiding this comment

avdg81 May 12, 2023

Choose a reason for hiding this comment

rubenzorrilla May 12, 2023

Choose a reason for hiding this comment

avdg81 commented May 12, 2023 • edited Loading

philbucher commented May 12, 2023

loumalouomega commented May 12, 2023

rubenzorrilla commented May 15, 2023

avdg81 commented May 15, 2023

loumalouomega commented May 15, 2023

philbucher left a comment

Choose a reason for hiding this comment

philbucher left a comment

Choose a reason for hiding this comment

avdg81 commented May 12, 2023 •

edited

Loading