Geo/line piping output #12852

WPK4FEM · 2024-11-15T15:42:30Z

📝 Description
Added

output items PIPE_ACTIVE, PIPE_HEIGHT, FLUID_FLUX_VECTOR, LOCAL_FLUID_FLUX_VECTOR, PERMEABILITY_MATRIX, LOCAL_PERMEABILITY_MATRIX for the line piping element
unit tests for those output items.

…Multiphysics/Kratos into geo/line_piping_output_2D

markelov208

Hi @WPK4FEM , I like how you made this PR because it is very understandable. Just a few comments.

markelov208 · 2024-11-18T09:33:53Z

applications/GeoMechanicsApplication/custom_elements/geo_steady_state_Pw_piping_element.h

+                this->GetGeometry().IntegrationPointsNumber(this->GetIntegrationMethod());
+            rValues.resize(number_of_integration_points);
+            auto        dynamic_viscosity_inverse = 1.0 / this->GetProperties()[DYNAMIC_VISCOSITY];
+            auto        constitutive_matrix = FillPermeabilityMatrix(this->GetValue(PIPE_HEIGHT));


Is it better to name it as permeability_matrix?

markelov208 · 2024-11-18T09:36:54Z

applications/GeoMechanicsApplication/custom_elements/geo_steady_state_Pw_piping_element.h

+                this->GetGeometry().IntegrationPoints(this->GetIntegrationMethod());
+            for (std::size_t i = 0; i < number_of_integration_points; ++i) {
+                // this should be rotated to the direction of the element for the global fluid flux vector
+                const auto head_gradient = CalculateHeadGradient(this->GetProperties(), this->GetGeometry());


It looks like head_gradient is calculated for the element. If this is correct, then it can be calculated together with longitudinal_flux before the for loop.

In the current situation and for the 2 noded element you are very correct here. I did it because on a 3 node ( maybe to be created in fulure ) element it would be varying over the element.

Changed to once per element.

markelov208 · 2024-11-18T09:58:21Z

applications/GeoMechanicsApplication/custom_elements/geo_steady_state_Pw_piping_element.h

+                if (rVariable == LOCAL_FLUID_FLUX_VECTOR) {
+                    rValues[i][0] = longitudinal_flux;
+                    rValues[i][1] = 0.0;
+                    rValues[i][2] = 0.0;
+                } else {
+                    Matrix jacobian;
+                    this->GetGeometry().Jacobian(jacobian, r_integration_points[i]);
+                    const auto tangential_vector =
+                        GeoMechanicsMathUtilities::Normalized(Vector{column(jacobian, 0)});
+                    rValues[i][0] = longitudinal_flux * tangential_vector[0];
+                    rValues[i][1] = longitudinal_flux * tangential_vector[1];
+                    rValues[i][2] = longitudinal_flux * tangential_vector[2];
+                }


Would it be clearer to have the for loop (or std::fill) inside if statements? If longitudinal_flux is the same for all points, then values are a tangential vector multiplied by the flux. In case of LOCAL_FLUID_FUX_VECTOR, the tangentional_vector is (1,0,0)

markelov208 · 2024-11-18T10:04:57Z

applications/GeoMechanicsApplication/custom_elements/geo_steady_state_Pw_piping_element.h

@@ -241,7 +318,7 @@ class KRATOS_API(GEO_MECHANICS_APPLICATION) GeoSteadyStatePwPipingElement : publ
        return result;
    }

-    Matrix FillPermeabilityMatrix(double pipe_height) const
+    static Matrix FillPermeabilityMatrix(double pipe_height)
    {
        return ScalarMatrix{1, 1, std::pow(pipe_height, 3) / 12.0};


Just for my knowledge, it should be power of 4 multiplied by a pipe factor, does it mean the factor is dimensional now? Thank you.

Apparently yes, Jon and myself share your questioning.

markelov208 · 2024-11-18T10:38:41Z

...echanicsApplication/tests/cpp_tests/custom_elements/test_geo_steady_state_piping_element.cpp

+        CreateHorizontalUnitLengthGeoSteadyStatePwPipingElementWithPWDofs(r_model_part, p_properties);
+    p_element->GetProperties().SetValue(DENSITY_WATER, 1.0E3);
+    p_element->GetProperties().SetValue(DYNAMIC_VISCOSITY, 1.0E-2);
+    p_element->GetProperties().SetValue(PIPE_HEIGHT, 1.0E-1);


Sorry I forgot the difference between GetProperties().SetValue(PIPE_HEIGHT) and SetValue(PIPE_HEIGHT). Should this line be removed?

The first writes to the material properties, the last directly to the element. Only the last is used, so I removed the first. Good catch!

markelov208 · 2024-11-18T10:39:48Z

...echanicsApplication/tests/cpp_tests/custom_elements/test_geo_steady_state_piping_element.cpp

+    p_element->CalculateOnIntegrationPoints(LOCAL_FLUID_FLUX_VECTOR, fluid_fluxes, dummy_process_info);
+
+    // Assert
+    KRATOS_EXPECT_DOUBLE_EQ(fluid_fluxes[0](0), 1.E-3 * 0.1 / 12.0);


Should 1.E-3 and 0.1 have named constants?

avdg81

A clear and very useful extension of the new piping element. I'm sure some people will be very pleased to have your changes available. All of my suggestions are minor in nature and non-blocking.

applications/GeoMechanicsApplication/custom_elements/geo_steady_state_Pw_piping_element.h

...echanicsApplication/tests/cpp_tests/custom_elements/test_geo_steady_state_piping_element.cpp

applications/GeoMechanicsApplication/custom_elements/geo_steady_state_Pw_piping_element.h

- Restored a variable name that was accidentally reverted. - Check all entries of returned vectors. - Fixed a typo.

avdg81

Thank you for processing the review comments and replying to the questions. I believe this PR is ready to be merged.

WPK4FEM added 5 commits November 14, 2024 13:47

CalculateOnIntegrationPoints first attempt + unit test for PIPE_ACTIVE

7d8888d

Unit test for PIPE_HEIGHT added

810c289

Unit test for permeability constitutive tensor.

5f55bc6

std::fill_n and unit test for calculateOnIntegrationpoints of flux

ab8f3f6

SonarCloud + Clang Tidy remarks, Global flux output.

d0a3927

WPK4FEM requested review from rfaasse, markelov208 and avdg81 November 15, 2024 15:42

WPK4FEM self-assigned this Nov 15, 2024

WPK4FEM added 7 commits November 15, 2024 17:14

CalculateOnIntegrationPoints first attempt + unit test for PIPE_ACTIVE

9588a5b

Unit test for PIPE_HEIGHT added

93608a4

Unit test for permeability constitutive tensor.

4b9c6e9

std::fill_n and unit test for calculateOnIntegrationpoints of flux

b13edcb

SonarCloud + Clang Tidy remarks, Global flux output.

53af7ca

std::size_t to avoid build warning that are treated as errors.

ed39082

Merge branch 'geo/line_piping_output_2D' of https://github.com/Kratos…

f0fe339

…Multiphysics/Kratos into geo/line_piping_output_2D

markelov208 reviewed Nov 18, 2024

View reviewed changes

avdg81 reviewed Nov 18, 2024

View reviewed changes

WPK4FEM and others added 4 commits November 18, 2024 14:29

First of review comments from Gennady taken up.

6d28134

Most of review suggestions by Anne.

ef5ec51

Refactored the calculation of the fluid flux vector

f93e045

Minor fixes and extensions

047d2c1

- Restored a variable name that was accidentally reverted. - Check all entries of returned vectors. - Fixed a typo.

WPK4FEM requested a review from avdg81 November 19, 2024 08:04

avdg81 approved these changes Nov 19, 2024

View reviewed changes

WPK4FEM merged commit f9fcc60 into master Nov 19, 2024
11 checks passed

WPK4FEM deleted the geo/line_piping_output_2D branch November 19, 2024 09:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Geo/line piping output #12852

Geo/line piping output #12852

WPK4FEM commented Nov 15, 2024

markelov208 left a comment

markelov208 Nov 18, 2024

WPK4FEM Nov 18, 2024

markelov208 Nov 18, 2024

WPK4FEM Nov 18, 2024

markelov208 Nov 18, 2024

markelov208 Nov 18, 2024

WPK4FEM Nov 18, 2024

markelov208 Nov 18, 2024

WPK4FEM Nov 18, 2024

markelov208 Nov 18, 2024

avdg81 left a comment

avdg81 left a comment

Geo/line piping output #12852

Geo/line piping output #12852

Conversation

WPK4FEM commented Nov 15, 2024

markelov208 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

avdg81 left a comment

Choose a reason for hiding this comment

avdg81 left a comment

Choose a reason for hiding this comment