[GeoMechanicsApplication] Add normal heat flux boundary condition #11783

markelov208 · 2023-11-09T12:52:22Z

📝 Description
A brief description of the PR.

Please mark the PR with appropriate tags:

Normal heat flux condition is added (TNormalFluxCondition).
A thermal condition class is added which include the basic functions of thermal conditions (TCondition).

🆕 Changelog
Please summarize the changes in one list to generate the changelog:
E.g.

added relevant files from Branch: https://github.com/KratosMultiphysics/Kratos/tree/geo/10372-HeatTransfer-temp
added unit tests for GetDofList and EquationIdVector function in TNormalFluxCondition
added acceptance tests for TNormalFluxCondition from Branch: https://github.com/KratosMultiphysics/Kratos/tree/geo/10372-HeatTransfer-temp but their calls are commented because t solver has not been added yet.

- added tests for these classes

added acceptance tests

# Conflicts: # applications/GeoMechanicsApplication/tests/test_GeoMechanicsApplication.py

# Conflicts: # applications/GeoMechanicsApplication/geo_mechanics_application.cpp # applications/GeoMechanicsApplication/geo_mechanics_application.h

rfaasse

Thank you for adding this code, it looks clean and readable!

I have some comments, mostly related to style and minor improvements. A proposition with some more work related to it would be adding more validations in the unit tests, instead of checking just one of the functions (I think it should be doable, but we can have a look!).

applications/GeoMechanicsApplication/custom_conditions/T_condition.cpp

rfaasse · 2023-11-20T12:50:11Z

applications/GeoMechanicsApplication/custom_conditions/T_condition.cpp

+    if (rLeftHandSideMatrix.size1() != TNumNodes || rLeftHandSideMatrix.size2() != TNumNodes) {
+        rLeftHandSideMatrix.resize(TNumNodes, TNumNodes, false);
+    }
+    noalias(rLeftHandSideMatrix) = ZeroMatrix(TNumNodes, TNumNodes);


Is it necessary to first resize the matrix before assigning a ZeroMatrix with a certain size? Same holds for the rRightHandSideMatrix

I have checked with the debugger. Yes, we have to resize before noalias because the latter does not change the matrix size. Without noalias we do not need this resize. Shall we avoid noalias?

I don't know enough about the advantages of noalias. It might be more efficient, but I don't know if that's the case here as well. Do you have a suggestion @avdg81?

My understanding is that the primary aim of the noalias syntax is to eliminate temporaries where the programmer sees this can be done safely. From the ublas documentation (see the Section named "Aliases"):

Abstract formulas on vectors and matrices normally compose a couple of unary and binary operations. The conventional way of evaluating such a formula is first to evaluate every leaf operation of a composition into a temporary and next to evaluate the composite resulting in another temporary. This method is expensive in terms of time especially for small and space especially for large vectors and matrices. [...]

One may get serious side effects using element wise evaluation on vectors or matrices. [...]

Our solution for this problem (i.e. where during the evaluation elements are overwritten that affect other evaluations) is to evaluate the right hand side of an assignment into a temporary and then to assign this temporary to the left hand side. [...] By using this syntax (i.e. noalias) a programmer can confirm, that the left and right hand sides of an assignment are independent, so that element wise evaluation and direct assignment to the target is safe.

The documentation doesn't mention that using noalias prevents resizing of vectors and matrices, as observed by @markelov208. I need to investigate that behavior in more detail first before making any statements about that. However, for this particular case, where we assign a zero matrix, I don't directly see any benefit in using noalias. Therefore, I would suggest to remove the noalias here, as well as the preceding resizing operation, since it is confusing and not strictly necessary. Do you agree @markelov208 and @rfaasse?

I agree with removing noalias in that case

I removed resize and noalias

applications/GeoMechanicsApplication/custom_conditions/T_condition.cpp

applications/GeoMechanicsApplication/custom_conditions/T_normal_flux_condition.cpp

applications/GeoMechanicsApplication/tests/test_t_normal_flux_condition.cpp

applications/GeoMechanicsApplication/tests/test_transient_thermal.py

- removed "start_time" from solver_settings - added Geo prefix

markelov208

Thank you Richard for the valuable comments. Hope all your comments are resolved now.

rfaasse

Thanks a lot for adding the extra unit tests, these are quite some work to figure out how to do it and how to test for the values, but this creates a nice safety net!

I have a few minor comments left, but nothing blocking

applications/GeoMechanicsApplication/tests/cpp_tests/test_t_normal_flux_condition.cpp

applications/GeoMechanicsApplication/tests/test_GeoMechanicsApplication.py

applications/GeoMechanicsApplication/custom_conditions/T_condition.cpp

applications/GeoMechanicsApplication/tests/cpp_tests/test_t_normal_flux_condition.cpp

…side

markelov208

Richard, once again thank you for the thorough review.

rfaasse

Thanks for all the hard work, it looks good to me now (but let's wait for the approval of @WPK4FEM as well)!

…COMPRESSIBILITY

markelov208

Richard and Wijtze Pieter, almost last changes. There are still two comments I would like to talk tomorrow. Thank you.

rfaasse

Nicely done! Ready for merge!

Gennady Markelov added 4 commits November 7, 2023 21:17

added T_condition/T_normal_flux_condition

9443286

- declared and registered instantiated classes for TnormalFluxCondition

bef3d1d

- added tests for these classes

added unit tests

2daf60e

added acceptance tests

commented calls to transient thermal tests

a98565f

markelov208 self-assigned this Nov 9, 2023

Gennady Markelov added 4 commits November 9, 2023 22:36

fixed unit tests

e809d7d

Merge branch 'master' into geo/11781-add-T-normal-flux-condition

49eed14

# Conflicts: # applications/GeoMechanicsApplication/tests/test_GeoMechanicsApplication.py

added using *Type

63e357f

applied suggestions from Codacy Production

9b54af3

markelov208 changed the title ~~[GeoMechanicsApplication] Add normal heat flux boundary condition #11781~~ [GeoMechanicsApplication] Add normal heat flux boundary condition Nov 10, 2023

Gennady Markelov added 3 commits November 10, 2023 15:14

added white spaces to trigger checks

0ed2a51

added using NodeType=Node;

245185b

reverted this->

fffa761

markelov208 requested a review from rfaasse November 20, 2023 09:07

Gennady Markelov added 2 commits November 20, 2023 11:26

Merge branch 'master' into geo/11781-add-T-normal-flux-condition

67c650c

# Conflicts: # applications/GeoMechanicsApplication/geo_mechanics_application.cpp # applications/GeoMechanicsApplication/geo_mechanics_application.h

re-formatting of TNormalFluxConidion*

87752c3

rfaasse reviewed Nov 20, 2023

View reviewed changes

Gennady Markelov added 13 commits November 20, 2023 15:08

local alphabetical order for include statements

ecc42d3

used Kratos::make_intrusive

5417bd9

Merge branch 'master' into geo/11781-add-T-normal-flux-condition

651023b

Merge branch 'master' into geo/11781-add-T-normal-flux-condition

a49e393

activated integration tests

a939ef8

updated integrations tests

5872c75

- removed "start_time" from solver_settings - added Geo prefix

corrected THERMAL_CONDUCTIVITY_SOLID_ZX

a515627

added GeoThemalDispersionLaw objects (does not compiled)

4183072

added objects of GeoThermalDispersionLaw

7c982e5

added default constructor

bb5f8a0

registered DT_TEMPERATURE_COEFFICIENT as python variable

e45f5da

changed Variables to variables

f90b4fe

changed local variables' name in accordance with the style guide

ab3f0fc

Gennady Markelov added 3 commits November 22, 2023 22:51

moved GetDofList from cpp to header

b519ada

used make_intrusive instead of new

d20f3d7

added tests for CalculateLocalSystem function

57ff99d

markelov208 commented Nov 22, 2023

View reviewed changes

Gennady Markelov added 2 commits November 23, 2023 00:03

fixed this->GetGeometry

422bca8

Merge branch 'master' into geo/11781-add-T-normal-flux-condition

5461473

rfaasse reviewed Nov 23, 2023

View reviewed changes

Gennady Markelov added 2 commits November 23, 2023 13:05

renamed RHS as rExpectedRightHandSide and rhs as expected_right_hand_…

78c17f9

…side

added const to the vector

b507eee

markelov208 commented Nov 23, 2023

View reviewed changes

added using Condition::Create; to avoid the code smells.

4dc878d

rfaasse previously approved these changes Nov 23, 2023

View reviewed changes

Gennady Markelov added 6 commits November 23, 2023 18:41

added Geo prefix to TNormalFluxCondition and TCondition

0596675

removed resize and noalias

a740d5a

changed NormalFlux and FluxVector

5eb655e

removed LONGITUDINAL_DISPERSIVITY, TRANSVERSE_DISPERSIVITY and SOLID_…

1fe7b84

…COMPRESSIBILITY

removed fluid_pressure_type

f34e782

removed .gid extension to folders

d33d69a

markelov208 dismissed rfaasse’s stale review via d33d69a November 23, 2023 19:19

markelov208 commented Nov 23, 2023

View reviewed changes

replaced SATURATION with SATURATED_SATURATION

d60d384

markelov208 requested a review from a team as a code owner November 24, 2023 12:40

Gennady Markelov added 2 commits November 24, 2023 13:40

Merge branch 'master' into geo/11781-add-T-normal-flux-condition

5d662df

reverted SATURATION

b34582f

markelov208 removed the request for review from a team November 24, 2023 13:16

WPK4FEM linked an issue Nov 24, 2023 that may be closed by this pull request

[GeoMechanicsApplication] Add normal heat flux boundary condition #11781

Closed

added saturation via a retention law

e3f419b

rfaasse approved these changes Nov 24, 2023

View reviewed changes

markelov208 merged commit 3048ca6 into master Nov 24, 2023
15 of 16 checks passed

markelov208 deleted the geo/11781-add-T-normal-flux-condition branch November 24, 2023 15:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GeoMechanicsApplication] Add normal heat flux boundary condition #11783

[GeoMechanicsApplication] Add normal heat flux boundary condition #11783

markelov208 commented Nov 9, 2023 •

edited

Loading

rfaasse left a comment

rfaasse Nov 20, 2023

markelov208 Nov 21, 2023

rfaasse Nov 22, 2023

avdg81 Nov 23, 2023

rfaasse Nov 23, 2023

markelov208 Nov 23, 2023

markelov208 left a comment

rfaasse left a comment

markelov208 left a comment

rfaasse left a comment

markelov208 left a comment

rfaasse left a comment

[GeoMechanicsApplication] Add normal heat flux boundary condition #11783

[GeoMechanicsApplication] Add normal heat flux boundary condition #11783

Conversation

markelov208 commented Nov 9, 2023 • edited Loading

rfaasse left a comment

Choose a reason for hiding this comment

rfaasse Nov 20, 2023

Choose a reason for hiding this comment

markelov208 Nov 21, 2023

Choose a reason for hiding this comment

rfaasse Nov 22, 2023

Choose a reason for hiding this comment

avdg81 Nov 23, 2023

Choose a reason for hiding this comment

rfaasse Nov 23, 2023

Choose a reason for hiding this comment

markelov208 Nov 23, 2023

Choose a reason for hiding this comment

markelov208 left a comment

Choose a reason for hiding this comment

rfaasse left a comment

Choose a reason for hiding this comment

markelov208 left a comment

Choose a reason for hiding this comment

rfaasse left a comment

Choose a reason for hiding this comment

markelov208 left a comment

Choose a reason for hiding this comment

rfaasse left a comment

Choose a reason for hiding this comment

markelov208 commented Nov 9, 2023 •

edited

Loading