[GeoMechanicsApplication] Clean up retention response #12392

avdg81 · 2024-05-21T11:16:12Z

📝 Description
Cleaned up several things related to the calculation of the retention response.

🆕 Changelog

When calculating the values at integration points, only call the relevant functions rather than CalculateRetentionResponse (which calculates all retention parameters). This saves us from calculating (and storing) unnecessary data.
Consequently, it was no longer needed to calculate the effective saturation as part of CalculateRetentionResponse.
Removed members FluidPressure and EffectiveSaturation from struct UPwSmallStrainElement::ElementVariables, since they had become redundant. Their usages have been replaced by adopting local variables. In a similar way, members FluidPressure and Density have been removed from struct SmallStrainUPwDiffOrderElement::ElementVariables.
Removed two unused using statements.

`CalculateRetentionResponse` calculates several parameters, even when not all of them are needed. In these two instances, it was clear how to replace these function calls, which makes the code easier to understand.

This eliminates two usages of member `BishopCoefficient` of struct `ElementVariables`.

Its usages could be replaced by using local variables.

It was assigned to only once, but never read from (due to earlier changes).

In this case, only the fluid pressure and the Bishop coefficient were needed.

Replaced all of its usages by local variables. Also replaced two usages of member `BishopCoefficient` by a local variable.

Its single usage has been replaced by a local variable.

WPK4FEM

Short and comprehensible. Remarks are improvementpoints not requirements.

WPK4FEM · 2024-05-21T15:23:47Z

applications/GeoMechanicsApplication/custom_elements/U_Pw_small_strain_element.cpp

+            if (rVariable == DEGREE_OF_SATURATION)
+                rOutput[GPoint] = mRetentionLawVector[GPoint]->CalculateSaturation(RetentionParameters);
+            if (rVariable == EFFECTIVE_SATURATION)
+                rOutput[GPoint] = mRetentionLawVector[GPoint]->CalculateEffectiveSaturation(RetentionParameters);
+            if (rVariable == BISHOP_COEFFICIENT)
+                rOutput[GPoint] = mRetentionLawVector[GPoint]->CalculateBishopCoefficient(RetentionParameters);
            if (rVariable == DERIVATIVE_OF_SATURATION)
-                rOutput[GPoint] = Variables.DerivativeOfSaturation;
+                rOutput[GPoint] =
+                    mRetentionLawVector[GPoint]->CalculateDerivativeOfSaturation(RetentionParameters);
            if (rVariable == RELATIVE_PERMEABILITY)
-                rOutput[GPoint] = Variables.RelativePermeability;
+                rOutput[GPoint] =
+                    mRetentionLawVector[GPoint]->CalculateRelativePermeability(RetentionParameters);


These are mutually exclusive.

Yes, I agree with your observation. I have changed the other branches to use else if. I would, however, argue that it would be better still to have such selections one level higher. It is no longer needed to group these variables, since we no longer call CalculateRetentionResponse. But I would suggest to do that another time.

WPK4FEM · 2024-05-21T15:29:05Z

applications/GeoMechanicsApplication/custom_elements/U_Pw_small_strain_element.cpp

            noalias(TotalStressVector) = mStressVector[GPoint];
            noalias(TotalStressVector) += PORE_PRESSURE_SIGN_FACTOR * Variables.BiotCoefficient *
-                                          Variables.BishopCoefficient * Variables.FluidPressure * VoigtVector;
+                                          bishop_coefficient * fluid_pressure * VoigtVector;


sig_tot = sig_eff + p * I
is computed here. That is probably clearer written down in 1 statement.

Yes, again I agree with you. I have made this calculation more straightforward and I've tried to take out any unnecessary intermediate variables.

WPK4FEM · 2024-05-21T15:30:32Z

applications/GeoMechanicsApplication/custom_elements/U_Pw_small_strain_element.cpp

+        const auto fluid_pressure =
            GeoTransportEquationUtilities::CalculateFluidPressure(Variables.Np, Variables.PressureVector);
-        RetentionParameters.SetFluidPressure(Variables.FluidPressure);
+        RetentionParameters.SetFluidPressure(fluid_pressure);


Why not like RetentionParameter.SetFluidPressure(GeoTransportEquationUtilities::CalculateFluidPressure(Variables.Np, Variables.PressureVector));

I thought it might be somewhat too long of an expression. But clearly, your perception is different, so I'll change it :-)

WPK4FEM · 2024-05-21T15:30:48Z

applications/GeoMechanicsApplication/custom_elements/U_Pw_small_strain_element.cpp

+    const auto fluid_pressure =
        GeoTransportEquationUtilities::CalculateFluidPressure(rVariables.Np, rVariables.PressureVector);
-    rRetentionParameters.SetFluidPressure(rVariables.FluidPressure);
+    rRetentionParameters.SetFluidPressure(fluid_pressure);



See previous comment.

WPK4FEM · 2024-05-21T15:33:31Z

applications/GeoMechanicsApplication/custom_elements/small_strain_U_Pw_diff_order_element.cpp

+            const auto fluid_pressure = GeoTransportEquationUtilities::CalculateFluidPressure(
                Variables.Np, Variables.PressureVector);
-            RetentionParameters.SetFluidPressure(Variables.FluidPressure);
+            RetentionParameters.SetFluidPressure(fluid_pressure);



Also 3 of these changes here in this file ( see 1033, 1167, 2957). If possible, do it in 1 statement.

rfaasse

This is a very nice and straight-forward PR and a clear improvement, I don't have anything to add 😄

Probably we can slowly but surely stop using the CalculateRetentionResponse everywhere, but this is a nice start!

- Made some selections mutually exclusive. - Eliminated a few intermediate variables. - Made the calculation of the total stress vector more straightforward. - Removed a few unnecessary resize operations.

WPK4FEM

Fine for me.

rfaasse

Ready to go!

avdg81 added 8 commits May 21, 2024 09:31

Replaced two calls to CalculateRetentionResponse by more specific ones

7ddb7b5

`CalculateRetentionResponse` calculates several parameters, even when not all of them are needed. In these two instances, it was clear how to replace these function calls, which makes the code easier to understand.

Removed two redundant using statements

565f47e

Use a local variable for the Bishop coefficient when possible

fdf5ca2

This eliminates two usages of member `BishopCoefficient` of struct `ElementVariables`.

Eliminated member FluidPressure of struct ElementVariables

2eb434e

Its usages could be replaced by using local variables.

Removed member EffectiveSaturation of struct ElementVariables

63a6b30

It was assigned to only once, but never read from (due to earlier changes).

Replaced a call to CalculateRetentionResponse by a more specific one

f650827

In this case, only the fluid pressure and the Bishop coefficient were needed.

Removed member FluidPressure from struct ElementVariables

17afc1b

Replaced all of its usages by local variables. Also replaced two usages of member `BishopCoefficient` by a local variable.

Removed member Density from struct ElementVariables

191efc5

Its single usage has been replaced by a local variable.

avdg81 requested review from rfaasse and WPK4FEM May 21, 2024 11:16

avdg81 self-assigned this May 21, 2024

Fixed GCC builds by adding this-> to CalculateRetentionResponse

9cf87eb

WPK4FEM previously approved these changes May 21, 2024

View reviewed changes

rfaasse previously approved these changes May 22, 2024

View reviewed changes

Processed Wijtze Pieter's review comments

85da166

- Made some selections mutually exclusive. - Eliminated a few intermediate variables. - Made the calculation of the total stress vector more straightforward. - Removed a few unnecessary resize operations.

avdg81 dismissed stale reviews from rfaasse and WPK4FEM via 85da166 May 22, 2024 08:26

avdg81 requested a review from WPK4FEM May 22, 2024 08:28

WPK4FEM approved these changes May 22, 2024

View reviewed changes

avdg81 enabled auto-merge (squash) May 22, 2024 09:56

rfaasse approved these changes May 22, 2024

View reviewed changes

avdg81 merged commit fd1f38f into master May 22, 2024
11 checks passed

avdg81 deleted the geo/clean-up-retention-response branch May 22, 2024 10:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GeoMechanicsApplication] Clean up retention response #12392

[GeoMechanicsApplication] Clean up retention response #12392

avdg81 commented May 21, 2024

WPK4FEM left a comment

WPK4FEM May 21, 2024

avdg81 May 22, 2024

WPK4FEM May 21, 2024

avdg81 May 22, 2024

WPK4FEM May 21, 2024

avdg81 May 22, 2024

WPK4FEM May 21, 2024

avdg81 May 22, 2024

WPK4FEM May 21, 2024

avdg81 May 22, 2024

rfaasse left a comment

WPK4FEM left a comment

rfaasse left a comment

[GeoMechanicsApplication] Clean up retention response #12392

[GeoMechanicsApplication] Clean up retention response #12392

Conversation

avdg81 commented May 21, 2024

WPK4FEM left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfaasse left a comment

Choose a reason for hiding this comment

WPK4FEM left a comment

Choose a reason for hiding this comment

rfaasse left a comment

Choose a reason for hiding this comment