[GeoMechanicsApplication] Implement well constitutive behaviour for line thermal element #12370

mnabideltares · 2024-05-10T23:17:39Z

📝 Description
To enhance our computational model for simulating high capacity heat storage systems, we need to implement a thermal "wel/filter" element on top of the existing thermal line element. The thermal filter element will enable us to model the behavior of heat storage under the ground, accounting for their heat exchange processes. This addition is crucial for achieving realistic simulations of heat storage systems and optimizing their performance.

🆕 Changelog

Added constutative model for the filter element
Added test cases

markelov208

Hi Mohamed, thank you for the new feature. Sorry I have a few comments and a general question why is it called 'ThermalFilter'?

applications/GeoMechanicsApplication/custom_constitutive/thermal_filter_law.cpp

...ons/GeoMechanicsApplication/tests/test_thermal_element/test_thermal_filter_element/README.md

applications/GeoMechanicsApplication/tests/test_transient_thermal.py

applications/GeoMechanicsApplication/custom_constitutive/thermal_filter_law.h

applications/GeoMechanicsApplication/geo_mechanics_application.h

markelov208

Hi Mohamed, concerning the Filter, let's keep it until @WPK4FEM has a better suggestion.

applications/GeoMechanicsApplication/custom_constitutive/thermal_filter_law.cpp

WPK4FEM

Hi Mohammed,
I have some trouble understanding the test. A Dirichlet condition at the same place will dictate the solution, and as such prevent testing of the constitutive behaviour in the line thermal element.

Also the constitutive law only looks at the conductivity of water, If that is all that is used, much of the given input for the filter element can be removed.

Regards, Wijtze Pieter

applications/GeoMechanicsApplication/custom_constitutive/thermal_filter_law.cpp

applications/GeoMechanicsApplication/custom_constitutive/thermal_filter_law.h

...icsApplication/tests/test_thermal_element/documentation_data/test_thermal_filter_element.svg

...cation/tests/test_thermal_element/test_thermal_filter_element/Common/MaterialParameters.json

...ons/GeoMechanicsApplication/tests/test_thermal_element/test_thermal_filter_element/README.md

...hermal_filter_element/test_thermal_filter_element_2D2N/test_thermal_filter_element_2D2N.mdpa

...hermal_filter_element/test_thermal_filter_element_2D3N/test_thermal_filter_element_2D3N.mdpa

rfaasse

Clear PR and nice integration test cases with clear documentation. I think it would be nice to add unit tests for the new class as well. Since it seems not a lot of setup is necessary, this should be doable. Other than that, I only have a few very minor remarks,

applications/GeoMechanicsApplication/custom_constitutive/thermal_filter_law.cpp

...on/tests/test_thermal_element/documentation_data/test_thermal_filter_element_2D3N_result.png

applications/GeoMechanicsApplication/custom_constitutive/thermal_filter_law.cpp

rfaasse

No blocking comments from my side, thanks for making the improvements and adding the tests! I have a few suggestions you may or may not want to take up.

rfaasse · 2024-05-14T12:06:53Z

applications/GeoMechanicsApplication/tests/cpp_tests/test_thermal_filter_law.cpp

+KRATOS_TEST_CASE_IN_SUITE(GetWorkingSpaceDimension_ReturnsFilterDimensionValue, KratosGeoMechanicsFastSuite)
+{
+    constexpr SizeType dimension = 1;
+    GeoThermalDispersionLaw geo_thermal_filter_law(dimension);
+
+    KRATOS_EXPECT_EQ(geo_thermal_filter_law.WorkingSpaceDimension(), dimension);
+}


We could add a simple test to check that the construction throws when the dimension is wrong by using KRATOS_EXPECT_EXCEPTION_IS_THROWN, see e.g.

Kratos/applications/GeoMechanicsApplication/tests/cpp_tests/test_variables_utilities.cpp

Lines 26 to 31 in 1166a00

KRATOS_TEST_CASE_IN_SUITE(TestVariablesUtilitiesThrowsWhenComponentDoesNotExist, KratosGeoMechanicsFastSuite)

{

KRATOS_EXPECT_EXCEPTION_IS_THROWN(

VariablesUtilities::GetComponentFromVectorVariable(ACCELERATION.Name(), "?"),

"Error: The component \"ACCELERATION_?\" is not registered!")

}

I don't consider this blocking, but it might be a nice test to add.

applications/GeoMechanicsApplication/tests/cpp_tests/test_thermal_filter_law.cpp

applications/GeoMechanicsApplication/geo_mechanics_application.h

… and Filter laws changed inheritance to GeoThermalLaw Test cases are modified, as the constutative law for heat is removed, and replaced by thermal law

fixed code smell

rfaasse

Thank you for your hard work on this Mohamed! It's great to see we can read it from json in this way.

I have a few minor comments left, but nothing blocking. However, since I was quite involved in some of the ideas, I will not approve the PR and prefer for someone else to do this in the end (since I'm of course a bit biased).

applications/GeoMechanicsApplication/custom_elements/transient_thermal_element.h

applications/GeoMechanicsApplication/custom_constitutive/thermal_law.h

applications/GeoMechanicsApplication/custom_constitutive/thermal_dispersion_law.h

avdg81

This is a very nice extension to the work that you have done for geothermal analysis. Although the basis of your extension is good, I still have a few suggestions that would make the extension even simpler and cleaner. Hope you find my feedback useful.

applications/GeoMechanicsApplication/custom_constitutive/thermal_dispersion_law.h

applications/GeoMechanicsApplication/custom_constitutive/thermal_law.h

applications/GeoMechanicsApplication/custom_constitutive/thermal_filter_law.h

applications/GeoMechanicsApplication/custom_constitutive/thermal_law.h

applications/GeoMechanicsApplication/tests/cpp_tests/test_thermal_filter_law.cpp

avdg81 · 2024-05-16T12:18:19Z

...MechanicsApplication/tests/test_thermal_element/test_micro_climate_1/MaterialParameters.json

@@ -25,7 +22,8 @@
        "ALPHA_COEFFICIENT"                : 1.900000e-01,
        "QF_COEFFICIENT"                   : 0.000000e+00,
        "SMIN_COEFFICIENT"                 : 0.000000e+00,
-        "SMAX_COEFFICIENT"                 : 4.800000e-04
+        "SMAX_COEFFICIENT"                 : 4.800000e-04,
+        "THERMAL_LAW"                      : "GeoThermalDispersionLaw"


Since this input parameter determines which other parameters are needed, I would put this as the very first one in the list.

avdg81

After your latest changes I'm very satisfied about this PR. Note that I have only a few minor remarks left. Please, also have a look at the code smells reported by SonarQube (there are 3 left, which are quite easy to fix). Thank you.

P.S.: You may want to wait with merging until @WPK4FEM has had another look.

avdg81 · 2024-05-17T07:27:44Z

applications/GeoMechanicsApplication/custom_constitutive/thermal_filter_law.h

+    void save(Serializer&) const override {}
+
+    void load(Serializer&) override {}


Please add comments to these function bodies which explain why they are empty. For instance, "Nothing to serialize, since there are no data members, and its base class is abstract". This will resolve two code smells found by SonarQube.

avdg81 · 2024-05-17T07:34:34Z

...ons/GeoMechanicsApplication/tests/test_thermal_element/test_thermal_filter_element/README.md

+**Source files:** [Thermal filter element with fixed temperature](https://github.com/KratosMultiphysics/Kratos/tree/master/applications/GeoMechanicsApplication/tests/test_thermal_element/test_thermal_filter_element)
+
+## Case Specification
+In this thermal test case, a $2 \mathrm{[m]} \times 1 \mathrm{[m]}$ soil block is considered, with everywhere set to 0 $\mathrm{[^\circ C]}$. A well/filter passes vertically through middle of the domain. Heat fluxes are set at the top and bottom of the filter, with values of 100 $\mathrm{[W/m^2]}$. Dirichlet boundary donditions of temperature are set at the left and right of the soil domain, both with a value of 0 $\mathrm{[^\circ C]}$. The simulation spans 100 days to allow for a transition from the initial to a linear temperature profile. This test is conducted for various configurations, including 2D3N and 2D6N soil elements (with corresponding 2D2N and 2D3N filter elements). The temperature distribution is then evaluated with its own results. The boundary conditions are shown below:


Typo: "donditions" => "conditions"

Like m and \circ C, days indicates a unit and should have [ ].

avdg81 · 2024-05-17T07:39:29Z

...n/tests/test_thermal_element/test_thermal_fixed_temperature/Common/MaterialParameters2D.json

@@ -17,7 +15,7 @@
              "THERMAL_CONDUCTIVITY_SOLID_XX" :  1.50,
              "THERMAL_CONDUCTIVITY_SOLID_YY" :  1.50,
              "THERMAL_CONDUCTIVITY_SOLID_XY" :  0.0,
-              "THERMAL_CONDUCTIVITY_WATER"    :  0.48
+              "THERMAL_CONDUCTIVITY_WATER"    :  0.48              


This trailing spaces were probably inserted accidentally.

WPK4FEM

Thank you for the tedious work, this looks good.

WPK4FEM · 2024-05-17T07:57:52Z

...ons/GeoMechanicsApplication/tests/test_thermal_element/test_thermal_filter_element/README.md

+**Source files:** [Thermal filter element with fixed temperature](https://github.com/KratosMultiphysics/Kratos/tree/master/applications/GeoMechanicsApplication/tests/test_thermal_element/test_thermal_filter_element)
+
+## Case Specification
+In this thermal test case, a $2 \mathrm{[m]} \times 1 \mathrm{[m]}$ soil block is considered, with everywhere set to 0 $\mathrm{[^\circ C]}$. A well/filter passes vertically through middle of the domain. Heat fluxes are set at the top and bottom of the filter, with values of 100 $\mathrm{[W/m^2]}$. Dirichlet boundary donditions of temperature are set at the left and right of the soil domain, both with a value of 0 $\mathrm{[^\circ C]}$. The simulation spans 100 days to allow for a transition from the initial to a linear temperature profile. This test is conducted for various configurations, including 2D3N and 2D6N soil elements (with corresponding 2D2N and 2D3N filter elements). The temperature distribution is then evaluated with its own results. The boundary conditions are shown below:


Like m and \circ C, days indicates a unit and should have [ ].

avdg81

I believe this is good to go. Thanks for the work done Mohamed!

mnabideltares added 4 commits May 7, 2024 09:51

Merge branch 'master' into geo/12322-implement-well-element

ab61bc8

Added constutative behaviour

ec7889a

Merge branch 'master' into geo/12322-implement-well-element

f684843

Added test cases for the filter element

22bbea7

mnabideltares linked an issue May 10, 2024 that may be closed by this pull request

[GeoMechanicsApplication] Implement well constitutive behaviour for line thermal element #12322

Closed

mnabideltares self-assigned this May 10, 2024

mnabideltares added the Feature label May 10, 2024

mnabideltares requested review from avdg81, rfaasse, WPK4FEM and markelov208 May 10, 2024 23:19

mnabideltares added 3 commits May 11, 2024 01:35

A fix in test case

12a2a88

README.md is added for the test cases

888f3af

Fix in readme

507de31

markelov208 reviewed May 11, 2024

View reviewed changes

modifications based on review

a318f95

markelov208 reviewed May 12, 2024

View reviewed changes

applications/GeoMechanicsApplication/custom_constitutive/thermal_filter_law.cpp Outdated Show resolved Hide resolved

WPK4FEM requested changes May 13, 2024

View reviewed changes

Modifications based on review comments

33eabb4

mnabideltares requested review from markelov208 and WPK4FEM May 13, 2024 12:30

Fix based on review

9550840

rfaasse reviewed May 13, 2024

View reviewed changes

fixes based on review comments

77a23f9

mnabideltares requested a review from rfaasse May 13, 2024 14:07

mnabideltares added 2 commits May 13, 2024 16:28

fixes based or review

765bdbe

Fix in unit-test for thermal filter constitutive law

d09c3ba

rfaasse previously approved these changes May 14, 2024

View reviewed changes

Addressed review comments

6be0896

mnabideltares dismissed rfaasse’s stale review via 6be0896 May 14, 2024 13:54

mnabideltares added 2 commits May 14, 2024 16:00

clang format

370b2a4

THERMAL_LAW is added, GeoThermalLaw base class is defined. Dispersion…

8a45f84

… and Filter laws changed inheritance to GeoThermalLaw Test cases are modified, as the constutative law for heat is removed, and replaced by thermal law

mnabideltares requested a review from rfaasse May 15, 2024 19:00

mnabideltares added 4 commits May 15, 2024 23:59

In the case of missing thermal_law, the dispersion-law is set as default

03b58e8

fixed code smell

fixed code smells

7f2d444

fixes to avoid WError

06b93a9

Fixes for WError's

18493b1

rfaasse reviewed May 16, 2024

View reviewed changes

Addressed changes based on review

f5a2a2e

avdg81 requested changes May 16, 2024

View reviewed changes

Modifications based on review

9831df4

mnabideltares requested review from avdg81 May 16, 2024 15:13

avdg81 previously approved these changes May 17, 2024

View reviewed changes

WPK4FEM previously approved these changes May 17, 2024

View reviewed changes

minor fixes based on review

0a76620

mnabideltares dismissed stale reviews from WPK4FEM and avdg81 via 0a76620 May 17, 2024 10:27

minor fix for code smell

ff656e0

avdg81 approved these changes May 17, 2024

View reviewed changes

mnabideltares merged commit ecd736a into master May 17, 2024
11 checks passed

mnabideltares deleted the geo/12322-implement-well-element branch May 17, 2024 12:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GeoMechanicsApplication] Implement well constitutive behaviour for line thermal element #12370

[GeoMechanicsApplication] Implement well constitutive behaviour for line thermal element #12370

mnabideltares commented May 10, 2024

markelov208 left a comment

markelov208 left a comment

WPK4FEM left a comment

rfaasse left a comment

rfaasse left a comment

rfaasse May 14, 2024

rfaasse left a comment

avdg81 left a comment

avdg81 May 16, 2024

avdg81 left a comment

avdg81 May 17, 2024

avdg81 May 17, 2024

WPK4FEM May 17, 2024

avdg81 May 17, 2024

WPK4FEM left a comment

WPK4FEM May 17, 2024

avdg81 left a comment

	KRATOS_TEST_CASE_IN_SUITE(TestVariablesUtilitiesThrowsWhenComponentDoesNotExist, KratosGeoMechanicsFastSuite)
	{
	KRATOS_EXPECT_EXCEPTION_IS_THROWN(
	VariablesUtilities::GetComponentFromVectorVariable(ACCELERATION.Name(), "?"),
	"Error: The component \"ACCELERATION_?\" is not registered!")
	}

		void save(Serializer&) const override {}

		void load(Serializer&) override {}

[GeoMechanicsApplication] Implement well constitutive behaviour for line thermal element #12370

[GeoMechanicsApplication] Implement well constitutive behaviour for line thermal element #12370

Conversation

mnabideltares commented May 10, 2024

markelov208 left a comment

Choose a reason for hiding this comment

markelov208 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

rfaasse left a comment

Choose a reason for hiding this comment

rfaasse May 14, 2024

Choose a reason for hiding this comment

rfaasse left a comment

Choose a reason for hiding this comment

avdg81 left a comment

Choose a reason for hiding this comment

avdg81 May 16, 2024

Choose a reason for hiding this comment

avdg81 left a comment

Choose a reason for hiding this comment

avdg81 May 17, 2024

Choose a reason for hiding this comment

avdg81 May 17, 2024

Choose a reason for hiding this comment

WPK4FEM May 17, 2024

Choose a reason for hiding this comment

avdg81 May 17, 2024

Choose a reason for hiding this comment

WPK4FEM left a comment

Choose a reason for hiding this comment

WPK4FEM May 17, 2024

Choose a reason for hiding this comment

avdg81 left a comment

Choose a reason for hiding this comment