[GeoMechanicsApplication] Clean up / simplify the calculation of the permeability update factor by avdg81 · Pull Request #12372 · KratosMultiphysics/Kratos

avdg81 · 2024-05-13T09:45:18Z

📝 Description
Simplified and cleaned up the implementation of the members that compute the permeability update factor.

🆕 Changelog

Made the member functions const.
Use the return type to return the functions' result.
Removed redundant usages of else keywords.
Use std::pow rather than C-style pow.
No need to instantiate a class to call its static members.
Only pass what is needed to compute the permeability update factor.
Reduced the scope of a local variable.
Renamed local variables to comply with the Kratos Style Guide.

Use the member functions' return type rather than assigning it to a member of an output argument.

No longer pass a reference to an `ElementVariables` instance, but only a const-reference to a strain vector.

rfaasse

Small and clear PR which once more reduces our use of ElementVariables. Also, good catch with the std::pow!

rfaasse · 2024-05-13T10:02:44Z

applications/GeoMechanicsApplication/custom_elements/small_strain_U_Pw_diff_order_element.cpp

-        rVariables.PermeabilityUpdateFactor = pow(10.0, permLog10);
-    } else {
-        rVariables.PermeabilityUpdateFactor = 1.0;
+        return std::pow(10.0, permLog10);


Good catch, it seems that before it found a boost version (in corecrt_math.h) and now just uses cmath. It's good to keep this in mind indeed.

WPK4FEM

Hi Anne,
This touches on the same lines as Richards pull request for strain computation.
Please be careful when merging.
Regards, Wijtze Pieter

WPK4FEM · 2024-05-13T10:04:31Z

applications/GeoMechanicsApplication/custom_elements/U_Pw_small_strain_element.cpp

-        rVariables.PermeabilityUpdateFactor = 1.0;
+    if (const auto& r_prop = this->GetProperties(); r_prop[PERMEABILITY_CHANGE_INVERSE_FACTOR] > 0.0) {
+        const double InverseCK = r_prop[PERMEABILITY_CHANGE_INVERSE_FACTOR];
+        const double epsV      = StressStrainUtilities::CalculateTrace(rStrainVector);


This resolves a lot of created confusion about the meaning of the volumetric strain. There was no need to mingle it with names indicating stress.

WPK4FEM · 2024-05-13T10:09:44Z

applications/GeoMechanicsApplication/custom_elements/small_strain_U_Pw_diff_order_element.cpp

+double SmallStrainUPwDiffOrderElement::CalculatePermeabilityUpdateFactor(const Vector& rStrainVector) const
 {
    KRATOS_TRY

-    const PropertiesType& rProp = this->GetProperties();
-
-    if (rProp[PERMEABILITY_CHANGE_INVERSE_FACTOR] > 0.0) {
-        const double InverseCK = rProp[PERMEABILITY_CHANGE_INVERSE_FACTOR];
-        const double epsV      = StressStrainUtilities::CalculateTrace(rVariables.StrainVector);
-        const double ePrevious = rProp[POROSITY] / (1.0 - rProp[POROSITY]);
+    if (const auto& r_prop = this->GetProperties(); r_prop[PERMEABILITY_CHANGE_INVERSE_FACTOR] > 0.0) {
+        const double InverseCK = r_prop[PERMEABILITY_CHANGE_INVERSE_FACTOR];
+        const double epsV      = StressStrainUtilities::CalculateTrace(rStrainVector);
+        const double ePrevious = r_prop[POROSITY] / (1.0 - r_prop[POROSITY]);
        const double eCurrent  = (1.0 + ePrevious) * std::exp(epsV) - 1.0;
        const double permLog10 = (eCurrent - ePrevious) * InverseCK;
-        rVariables.PermeabilityUpdateFactor = pow(10.0, permLog10);
-    } else {
-        rVariables.PermeabilityUpdateFactor = 1.0;
+        return std::pow(10.0, permLog10);
    }

+    return 1.0;
+
    KRATOS_CATCH("")
 }


Repeated in several elements. I do not see element specific code, should this be a utility function. The strain dependency points towards equation of motion or stress_strain utilities. Its use points to the transport equation utilities.

Yes, I agree that we can move this to a separate utility function. I would suggest to do that in a separate PR, just to keep the history clear.

markelov208

Anne, nice work in getting rid of Variables and code smells. After the changes made, it looks that both elements use almost the same function. Is it useful to put it in a common place? Thank you.

markelov208 · 2024-05-13T10:10:20Z

applications/GeoMechanicsApplication/custom_elements/U_Pw_small_strain_element.cpp

+    if (const auto& r_prop = this->GetProperties(); r_prop[PERMEABILITY_CHANGE_INVERSE_FACTOR] > 0.0) {
+        const double InverseCK = r_prop[PERMEABILITY_CHANGE_INVERSE_FACTOR];
+        const double epsV      = StressStrainUtilities::CalculateTrace(rStrainVector);
+        const double ePrevious = r_prop[POROSITY] / (1.0 - r_prop[POROSITY]);
+        const double eCurrent  = (1.0 + ePrevious) * std::exp(epsV) - 1.0;
+        const double permLog10 = (eCurrent - ePrevious) * InverseCK;
+        return std::pow(10.0, permLog10);


It looks that both elements use the same function and a difference is only in this->GetProperties. Is it useful to move the function to utilities with r_Prop as input?

Yes, I agree that we can move this to a separate utility function. I would suggest to do that in a separate PR, just to keep the history clear. As you indicated, we'd better provide the element properties as an input parameter to that new utility function.

avdg81 added 9 commits May 13, 2024 10:55

Made two member functions const

9876bc3

Properly return the permeability update factor

3803369

Use the member functions' return type rather than assigning it to a member of an output argument.

Removed two redundant usages of the else keyword

da171e6

Use std::pow rather than old-style pow

2035fcd

Static members don't require an object to be invoked on

5e7b508

Only pass what is needed to compute the permeability update factor

fc72926

No longer pass a reference to an `ElementVariables` instance, but only a const-reference to a strain vector.

Eliminated a usage of member PermeabilityUpdateFactor

9f6d367

Reduced the scope of a local variable

9945c66

Renamed a local variable to comply with the Kratos Style Guide

2b52ecf

avdg81 added Cleanup GeoMechanics Issues related to the GeoMechanicsApplication labels May 13, 2024

avdg81 requested review from WPK4FEM, markelov208 and rfaasse May 13, 2024 09:45

avdg81 self-assigned this May 13, 2024

rfaasse approved these changes May 13, 2024

View reviewed changes

WPK4FEM reviewed May 13, 2024

View reviewed changes

markelov208 reviewed May 13, 2024

View reviewed changes

avdg81 merged commit 271b400 into master May 13, 2024

avdg81 deleted the geo/simplify-permeability-update-factor branch May 13, 2024 13:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GeoMechanicsApplication] Clean up / simplify the calculation of the permeability update factor#12372

[GeoMechanicsApplication] Clean up / simplify the calculation of the permeability update factor#12372
avdg81 merged 9 commits intomasterfrom
geo/simplify-permeability-update-factor

avdg81 commented May 13, 2024

Uh oh!

rfaasse left a comment

Uh oh!

rfaasse May 13, 2024

Uh oh!

WPK4FEM left a comment

Uh oh!

WPK4FEM May 13, 2024

Uh oh!

WPK4FEM May 13, 2024

Uh oh!

avdg81 May 13, 2024

Uh oh!

markelov208 left a comment •

edited

Loading

Uh oh!

markelov208 May 13, 2024

Uh oh!

avdg81 May 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

avdg81 commented May 13, 2024

Uh oh!

rfaasse left a comment

Choose a reason for hiding this comment

Uh oh!

rfaasse May 13, 2024

Choose a reason for hiding this comment

Uh oh!

WPK4FEM left a comment

Choose a reason for hiding this comment

Uh oh!

WPK4FEM May 13, 2024

Choose a reason for hiding this comment

Uh oh!

WPK4FEM May 13, 2024

Choose a reason for hiding this comment

Uh oh!

avdg81 May 13, 2024

Choose a reason for hiding this comment

Uh oh!

markelov208 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markelov208 May 13, 2024

Choose a reason for hiding this comment

Uh oh!

avdg81 May 13, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

markelov208 left a comment •

edited

Loading