[GeoMechanicsApplication] Format custom constitutive folder using clang format #12130

rfaasse · 2024-03-01T10:54:10Z

No description provided.

applications/GeoMechanicsApplication/.git-blame-ignore-revs

mnabideltares

@rfaasse
Thank you for the nice work on clang formatting.
The format of the code is improved a lot by clang.
I have few preferences in formatting the code. I commented some. However, it is matter of convention, agreement and discussion, which style to be prefered.

mnabideltares · 2024-03-04T13:42:59Z

applications/GeoMechanicsApplication/custom_constitutive/bilinear_cohesive_2D_law.cpp

+    rConstitutiveMatrix(0, 0) =
+        YieldStress / ((1.0 - DamageThreshold) * CriticalDisplacement) *
+        ((1.0 - mStateVariable) / mStateVariable -
+         StrainVector[0] * StrainVector[0] /
+             (CriticalDisplacement * CriticalDisplacement * mStateVariable * mStateVariable * mStateVariable));
+    rConstitutiveMatrix(1, 1) =
+        YieldStress / ((1.0 - DamageThreshold) * CriticalDisplacement) *
+        ((1.0 - mStateVariable) / mStateVariable -
+         StrainVector[1] * StrainVector[1] /
+             (CriticalDisplacement * CriticalDisplacement * mStateVariable * mStateVariable * mStateVariable));
+
+    rConstitutiveMatrix(0, 1) = -YieldStress * StrainVector[0] * StrainVector[1] /
+                                ((1.0 - DamageThreshold) * CriticalDisplacement * CriticalDisplacement *
+                                 CriticalDisplacement * mStateVariable * mStateVariable * mStateVariable);
+    rConstitutiveMatrix(1, 0) = rConstitutiveMatrix(0, 1);


In line 77 and 78: "YieldStress ...." is in the second line
Line 88: "-YieldStress ..." at the same line of "rConstitutiveMatrix(0, 1) ="
There is then inconsistency in formatting

clang-format works with penalties, so it could always be a bit inconsistent, because it depends on the situation and there are always trade-offs (penalties can be conflicting, e.g. breaking a line has a certain penalty and going over the character limit has another penalty). However, now we don't have a penalty for breaking after assignment. Let's discuss in the team if we'd like to add it. When adding a (high) penalty for this, most cases will continue on the same line after an assignment.

mnabideltares · 2024-03-04T13:43:57Z

applications/GeoMechanicsApplication/custom_constitutive/bilinear_cohesive_2D_law.cpp

+    rConstitutiveMatrix(0, 0) =
+        YieldStress / ((1.0 - DamageThreshold) * CriticalDisplacement) *
+        ((1.0 - mStateVariable) / mStateVariable -
+         StrainVector[0] * StrainVector[0] /
+             (CriticalDisplacement * CriticalDisplacement * mStateVariable * mStateVariable * mStateVariable));
+    rConstitutiveMatrix(1, 1) = YoungModulus / (DamageThreshold * CriticalDisplacement);


Same as previous comment. Inconsistency in format

mnabideltares · 2024-03-04T13:50:17Z

applications/GeoMechanicsApplication/custom_constitutive/bilinear_cohesive_2D_law.cpp

+        rConstitutiveMatrix(0, 1) =
+            -YieldStress * StrainVector[0] * StrainVector[1] /
+                ((1.0 - DamageThreshold) * CriticalDisplacement * CriticalDisplacement *
+                 CriticalDisplacement * mStateVariable * mStateVariable * mStateVariable) -
+            std::copysign(1.0, StrainVector[0]) * YoungModulus * FrictionCoefficient /
+                (DamageThreshold * CriticalDisplacement);


Prefer to start them from the same column.
for example: line 118 starts from 4 spaces after line 117, but line 117 hasn't priority/hirarchy above line 118.
line 120, std:: hasn't extra priority on line 119

The reason of the indent here, is because line 118 and 119 belong together, if that's not there it's hard to see where the parenthesis ends.

mnabideltares · 2024-03-04T13:52:21Z

applications/GeoMechanicsApplication/custom_constitutive/bilinear_cohesive_2D_law.cpp

+    rConstitutiveMatrix(0, 0) = YieldStress / (CriticalDisplacement * mStateVariable) *
+                                (1.0 - mStateVariable) / (1.0 - DamageThreshold);


In my opinion, it is a good format, and the rest need to follow this formatting

Yes and when the lines are not too long/complicated, this is what it will do. When it gets more complicated we'd need to introduce a penalty (see previous comment) for breaking a line after assignment

mnabideltares · 2024-03-04T13:54:38Z

applications/GeoMechanicsApplication/custom_constitutive/bilinear_cohesive_2D_law.cpp

+    rStressVector[indexStress2DInterface::INDEX_2D_INTERFACE_ZZ] =
+        YoungModulus / (DamageThreshold * CriticalDisplacement) *
+        StrainVector[indexStress2DInterface::INDEX_2D_INTERFACE_ZZ];


Prefer to have line 200 at the same line of 119, to keep consistency with the format of other parts

If we make PenaltyBreakAssignment high enough, this will be enforced (at cost of longer lines)

mnabideltares · 2024-03-04T15:52:32Z

applications/GeoMechanicsApplication/custom_constitutive/small_strain_umat_3D_law.cpp

+        KRATOS_INFO("Error in loadUMATWindows")
+            << "cannot load function umat in the specified UMAT: " << rMaterialProperties[UDSM_NAME]
+            << std::endl;


Same as previous

mnabideltares · 2024-03-04T15:53:54Z

applications/GeoMechanicsApplication/custom_constitutive/small_strain_umat_3D_law.cpp

+        }
+    } else {
+        for (unsigned int i = 0; i < VOIGT_SIZE_3D; i++) {
+            for (unsigned int j = 0; j < VOIGT_SIZE_3D; j++) {


}
else {
...
}

mnabideltares · 2024-03-04T15:55:49Z

applications/GeoMechanicsApplication/custom_constitutive/small_strain_umat_3D_law.cpp

 {
-    if (rThisVariable == NUMBER_OF_UMAT_STATE_VARIABLES) rValue = static_cast<int>(mStateVariablesFinalized.size());


if (rThisVariable == NUMBER_OF_UMAT_STATE_VARIABLES) {
rValue = static_cast(mStateVariablesFinalized.size());
}

mnabideltares · 2024-03-04T15:56:03Z

applications/GeoMechanicsApplication/custom_constitutive/small_strain_umat_3D_law.cpp

+    if (rThisVariable == NUMBER_OF_UMAT_STATE_VARIABLES)
+        rValue = static_cast<int>(mStateVariablesFinalized.size());


mnabideltares · 2024-03-04T15:57:04Z

applications/GeoMechanicsApplication/custom_constitutive/small_strain_umat_3D_law.cpp

+    } else if ((rThisVariable == CAUCHY_STRESS_VECTOR) && (rValue.size() == mStressVectorFinalized.size())) {
        std::copy(rValue.begin(), rValue.end(), mStressVectorFinalized.begin());


}
else if ...

…stom-constitutive-using-clang-format

rfaasse

Thank you for the thorough review and extensive comments @mnabideltares! I agree there are still improvement points and inconsistencies, which as discussed in the stand-up, I would like to take up point for point when all code is formatted:

We should discuss in the team if the majority prefers else on the same or next line with respect to the closing '}' of the if-statement
We can experiment and discuss about higher penalties on breaking after assignment operators (i.e. force a line to continue after '='.
We can experiment and discuss the breaking after '<<', which in combination with the KRATOS_ERROR is not always consistent now.
We can discuss how we want to handle indentation when breaking a line (remove it or keep it).

If you agree, I'd propose to tackle these 4 issues separately and merge this PR if there are no other blockers from your perspective.

rfaasse · 2024-03-05T10:36:42Z

applications/GeoMechanicsApplication/custom_constitutive/bilinear_cohesive_3D_law.cpp

+        if (Options.Is(ConstitutiveLaw::COMPUTE_CONSTITUTIVE_TENSOR)) {
            // COMPUTE_CONSTITUTIVE_TENSOR
            Matrix& rConstitutiveMatrix = rValues.GetConstitutiveMatrix();

-            if (EquivalentStrain >= mStateVariable) //Loading
+            if (EquivalentStrain >= mStateVariable) // Loading
            {
-                this->ComputeConstitutiveMatrixLoading(rConstitutiveMatrix,
-                                                       rStrainVector,


I agree, the issue here though is the // Loading comment. Because of the comment, clang-format cannot move the "{" to the same line. I would propose to keep it like this for the re-formatting (to not have a too large scope for this PR), but whenever we work on code that has these kinds of comments next to if-statements, we (re)move them

rfaasse · 2024-03-05T10:39:47Z

applications/GeoMechanicsApplication/custom_constitutive/bilinear_cohesive_3D_law.cpp

+            if (EquivalentStrain >= mStateVariable) // Loading
            {
-                this->ComputeConstitutiveMatrixContactLoading(rConstitutiveMatrix,


Same answer as previous, it's because of the comment

rfaasse · 2024-03-05T10:41:01Z

applications/GeoMechanicsApplication/custom_constitutive/bilinear_cohesive_3D_law.cpp

+            } else // Unloading
            {
-                this->ComputeConstitutiveMatrixContactUnloading(rConstitutiveMatrix,


We can discuss this in the team if we want to do the 'else' always on a new line. The "{ " on the same line is again an issue with the // Unloading comment

rfaasse · 2024-03-05T10:47:19Z

applications/GeoMechanicsApplication/custom_constitutive/bilinear_cohesive_2D_law.cpp

+    rConstitutiveMatrix(0, 0) =
+        YieldStress / ((1.0 - DamageThreshold) * CriticalDisplacement) *
+        ((1.0 - mStateVariable) / mStateVariable -
+         StrainVector[0] * StrainVector[0] /
+             (CriticalDisplacement * CriticalDisplacement * mStateVariable * mStateVariable * mStateVariable));
+    rConstitutiveMatrix(1, 1) =
+        YieldStress / ((1.0 - DamageThreshold) * CriticalDisplacement) *
+        ((1.0 - mStateVariable) / mStateVariable -
+         StrainVector[1] * StrainVector[1] /
+             (CriticalDisplacement * CriticalDisplacement * mStateVariable * mStateVariable * mStateVariable));
+
+    rConstitutiveMatrix(0, 1) = -YieldStress * StrainVector[0] * StrainVector[1] /
+                                ((1.0 - DamageThreshold) * CriticalDisplacement * CriticalDisplacement *
+                                 CriticalDisplacement * mStateVariable * mStateVariable * mStateVariable);
+    rConstitutiveMatrix(1, 0) = rConstitutiveMatrix(0, 1);


clang-format works with penalties, so it could always be a bit inconsistent, because it depends on the situation and there are always trade-offs (penalties can be conflicting, e.g. breaking a line has a certain penalty and going over the character limit has another penalty). However, now we don't have a penalty for breaking after assignment. Let's discuss in the team if we'd like to add it. When adding a (high) penalty for this, most cases will continue on the same line after an assignment.

rfaasse · 2024-03-05T10:50:34Z

applications/GeoMechanicsApplication/custom_constitutive/bilinear_cohesive_2D_law.cpp

+        rConstitutiveMatrix(0, 1) =
+            -YieldStress * StrainVector[0] * StrainVector[1] /
+                ((1.0 - DamageThreshold) * CriticalDisplacement * CriticalDisplacement *
+                 CriticalDisplacement * mStateVariable * mStateVariable * mStateVariable) -
+            std::copysign(1.0, StrainVector[0]) * YoungModulus * FrictionCoefficient /
+                (DamageThreshold * CriticalDisplacement);


The reason of the indent here, is because line 118 and 119 belong together, if that's not there it's hard to see where the parenthesis ends.

rfaasse · 2024-03-05T11:00:53Z

applications/GeoMechanicsApplication/custom_constitutive/small_strain_udsm_2D_interface_law.cpp

+        if (rValue.size() != VoigtSize) rValue.resize(VoigtSize);
+
+        rValue[INDEX_2D_INTERFACE_ZZ] = mStressVectorFinalized[INDEX_3D_ZZ];
+        rValue[INDEX_2D_INTERFACE_XZ] = mStressVectorFinalized[INDEX_3D_XZ];


clang-format does not add {}, so if we would like this we should do it ourselves. In the same way clang-format won't delete one blank line automatically (only if there are > 1, it will bring it back to 1 in our settings), there is still freedom to add these when needed.

I would propose to do these changes when we work in the code and encounter it (to limit the scope of this PR)

rfaasse · 2024-03-05T11:03:27Z

applications/GeoMechanicsApplication/custom_constitutive/small_strain_udsm_2D_interface_law.cpp

+    } else if ((rThisVariable == CAUCHY_STRESS_VECTOR) && (rValue.size() == VoigtSize)) {
+        this->SetInternalStressVector(rValue);
+    }


Let's discuss else/else if on a new line. I personally like to have it on the same to compact the code a bit, but let's see what the rest feels.

rfaasse · 2024-03-05T11:04:55Z

applications/GeoMechanicsApplication/custom_constitutive/small_strain_udsm_3D_law.cpp

-   for (unsigned int i = 0; i < VOIGT_SIZE_3D; ++i)
-      for (unsigned int j = 0; j < VOIGT_SIZE_3D; ++j)
-         mMatrixD[i][j] = rOther.mMatrixD[i][j];
+    for (unsigned int i = 0; i < VOIGT_SIZE_3D; ++i)
+        for (unsigned int j = 0; j < VOIGT_SIZE_3D; ++j)
+            mMatrixD[i][j] = rOther.mMatrixD[i][j];


Agreed, but as said before, clang-format won't add these automatically

rfaasse · 2024-03-05T11:05:29Z

applications/GeoMechanicsApplication/custom_constitutive/small_strain_udsm_3D_law.cpp

+    int                 iAbort                = 0;
+    auto                nSizeProjectDirectory = static_cast<int>(mProjectDirectory.size());


They are for aligning the assignments and variables

rfaasse · 2024-03-06T07:53:12Z

applications/GeoMechanicsApplication/custom_constitutive/bilinear_cohesive_3D_law.cpp

+    if (!rMaterialProperties.Has(CRITICAL_DISPLACEMENT) || rMaterialProperties[CRITICAL_DISPLACEMENT] <= 0.0)
+        KRATOS_ERROR << "CRITICAL_DISPLACEMENT has Key zero, is not defined or has an invalid "
+                        "value for property: "
+                     << rMaterialProperties.Id() << std::endl;
+
+    if (!rMaterialProperties.Has(YOUNG_MODULUS) || rMaterialProperties[YOUNG_MODULUS] <= 0.00)
+        KRATOS_ERROR
+            << "YOUNG_MODULUS has Key zero, is not defined or has an invalid value for property"
+            << rMaterialProperties.Id() << std::endl;


For the KRATOS_ERROR <<, Wijtze Pieter and I tried several settings to make it as readable as possible when formatting the elements. We landed on the current settings due to other inconsistencies that arise when putting a higher or lower penalty on breaking after the first <<. I'd propose to keep it like it is for now and after we did all the formatting, experiment a bit more, keeping in mind the entire geomechanics code base.

rfaasse · 2024-03-06T13:57:24Z

@mnabideltares I tried to answer all your (unique) questions in the comments, please check if you agree!

rfaasse added 2 commits March 1, 2024 11:52

Reformat custom constitutive

6c3bb88

Ignore previous format commit

be2875b

rfaasse requested a review from mnabideltares March 1, 2024 10:54

rfaasse commented Mar 1, 2024

View reviewed changes

applications/GeoMechanicsApplication/.git-blame-ignore-revs Show resolved Hide resolved

mnabideltares reviewed Mar 4, 2024

View reviewed changes

rfaasse added 2 commits March 5, 2024 11:18

Merge remote-tracking branch 'origin/master' into geo/11919-format-cu…

2c3b4f0

…stom-constitutive-using-clang-format

Merge remote-tracking branch 'origin/master' into geo/11919-format-cu…

9bed5b7

…stom-constitutive-using-clang-format

rfaasse commented Mar 6, 2024

View reviewed changes

rfaasse enabled auto-merge (squash) March 6, 2024 15:42

rfaasse linked an issue Mar 7, 2024 that may be closed by this pull request

[GeoMechanicsApplication] Format GeoMechanics code base using clang-format #11919

Closed

mnabideltares approved these changes Mar 7, 2024

View reviewed changes

rfaasse merged commit 3ff1502 into master Mar 7, 2024
16 of 17 checks passed

rfaasse deleted the geo/11919-format-custom-constitutive-using-clang-format branch March 7, 2024 10:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GeoMechanicsApplication] Format custom constitutive folder using clang format #12130

[GeoMechanicsApplication] Format custom constitutive folder using clang format #12130

rfaasse commented Mar 1, 2024

mnabideltares left a comment

mnabideltares Mar 4, 2024

rfaasse Mar 5, 2024

mnabideltares Mar 4, 2024

mnabideltares Mar 4, 2024

rfaasse Mar 5, 2024

mnabideltares Mar 4, 2024

rfaasse Mar 5, 2024

mnabideltares Mar 4, 2024

rfaasse Mar 5, 2024

mnabideltares Mar 4, 2024

mnabideltares Mar 4, 2024

mnabideltares Mar 4, 2024

mnabideltares Mar 4, 2024

mnabideltares Mar 4, 2024

rfaasse left a comment

rfaasse Mar 5, 2024

rfaasse Mar 5, 2024

rfaasse Mar 5, 2024

rfaasse Mar 5, 2024

rfaasse Mar 5, 2024

rfaasse Mar 5, 2024

rfaasse Mar 5, 2024

rfaasse Mar 5, 2024

rfaasse Mar 5, 2024

rfaasse Mar 6, 2024

rfaasse commented Mar 6, 2024

		rConstitutiveMatrix(0, 0) = YieldStress / (CriticalDisplacement * mStateVariable) *
		(1.0 - mStateVariable) / (1.0 - DamageThreshold);

		if (rThisVariable == NUMBER_OF_UMAT_STATE_VARIABLES)
		rValue = static_cast<int>(mStateVariablesFinalized.size());

		} else if ((rThisVariable == CAUCHY_STRESS_VECTOR) && (rValue.size() == mStressVectorFinalized.size())) {
		std::copy(rValue.begin(), rValue.end(), mStressVectorFinalized.begin());

		int iAbort = 0;
		auto nSizeProjectDirectory = static_cast<int>(mProjectDirectory.size());

[GeoMechanicsApplication] Format custom constitutive folder using clang format #12130

[GeoMechanicsApplication] Format custom constitutive folder using clang format #12130

Conversation

rfaasse commented Mar 1, 2024

mnabideltares 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

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

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 commented Mar 6, 2024