Skip to content

[GeoMechanicsApplication] Add thermal constitutive law #11747

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 14 commits into from
Nov 7, 2023
Merged

[GeoMechanicsApplication] Add thermal constitutive law #11747

merged 14 commits into from
Nov 7, 2023

Conversation

markelov208
Copy link
Contributor

@markelov208 markelov208 commented Nov 2, 2023 •
edited
Loading

📝 Description
A brief description of the PR.

  • added thermal constitutive law for the heat model

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

  • cleaning and refactoring thermal_dispersion_law.hpp/cpp files
  • added unit tests for 2D and 3D cases

Gennady Markelov and others added 6 commits October 31, 2023 15:29
- created, defined, registered INDEX_2D_THERMAL_FLUX_X/Y/Z
766601d 
- cleaned a bit thermal_dispersion_2D_law files.
- removed CLBaseType
85080fa 
- replace 2 with constexpr
replaced C with rThermalDispersionMatrix
8772c93
removed comment
26cdb72
@avdg81
Replaced the newly created variables by a plain enum
53a32ca 
Based on Mohamed's feedback, the newly created variables have been
replaced by the values of a new (plain) `enum`. Later on, we need to
replace the plain `enum` by a scoped `enum` (which is more modern and
safer to use).
added tests for CalculateThermalDispersionMatrix
3514bc3
@markelov208 markelov208 requested a review from rfaasse November 2, 2023 08:47
@markelov208 markelov208 linked an issue Nov 2, 2023 that may be closed by this pull request
[GeoMechanicsApplication] Add thermal constitutive law #11740
Closed
@markelov208 markelov208 changed the title Geo/11740 add thermal constitutive law [GeoMechanicsApplication] Add thermal constitutive law Nov 2, 2023
rfaasse
rfaasse requested changes Nov 2, 2023
View reviewed changes
Copy link
Contributor

@rfaasse rfaasse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clear and understandable code and great to see the unit tests are added.

applications/GeoMechanicsApplication/custom_constitutive/thermal_dispersion_2D_law.cpp Outdated
{
KRATOS_TRY

const double cWater = rProp[POROSITY] * rProp[SATURATION];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the SATURATION, @WPK4FEM proposed to investate if we could re-use the retention laws that are already there instead of having this new entry in the MaterialParameters.json. I think it would be a good idea to discuss this in office.

rfaasse
rfaasse previously approved these changes Nov 3, 2023
View reviewed changes
@rfaasse rfaasse requested a review from avdg81 November 3, 2023 14:54
avdg81
avdg81 reviewed Nov 3, 2023
View reviewed changes
Copy link
Contributor

@avdg81 avdg81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR looks nice and clean. I only have several minor comments and suggestions.

applications/GeoMechanicsApplication/tests/cpp_tests/test_thermal_dispersion_law.cpp Outdated
KRATOS_TEST_CASE_IN_SUITE(GetWorkingSpaceDimension_ReturnsCorrectValue, KratosGeoMechanicsFastSuite)
{
const SizeType dimension = 3;
GeoThermalDispersionLaw geo_thermal_dispersion_2D_law(dimension);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also rename this local variable for consistency:

Suggested change
GeoThermalDispersionLaw geo_thermal_dispersion_2D_law(dimension);
GeoThermalDispersionLaw geo_thermal_dispersion_3D_law(dimension);

And perhaps also add a check (in this test case) for the 2D case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

corrected

@markelov208
Copy link
Contributor Author

Anne, thank you for the comments. A new commit has just been done.

@markelov208 markelov208 self-assigned this Nov 6, 2023
avdg81
avdg81 previously approved these changes Nov 6, 2023
View reviewed changes
Copy link
Contributor

@avdg81 avdg81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really nice. Thanks for the hard work Gennady!

@markelov208
Copy link
Contributor Author

Hi Anne, thank you for the info on code smells and high esteem of my work.

avdg81
avdg81 approved these changes Nov 7, 2023
View reviewed changes
Copy link
Contributor

@avdg81 avdg81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're ready to merge. Please Gennady, feel free to press the button :-)

@markelov208
Copy link
Contributor Author

Good morning Anne, unfortunately there is a failed test, MeshMovingApplication. I think this is not GeoMechanics but it blocks the merge button. Is there a 'secret' button to accept failed tests? GitLab has it.

@avdg81
Copy link
Contributor

avdg81 commented Nov 7, 2023

Good morning Anne, unfortunately there is a failed test, MeshMovingApplication. I think this is not GeoMechanics but it blocks the merge button. Is there a 'secret' button to accept failed tests? GitLab has it.

Yes, we've noticed ;-) I have pressed the button to rerun the failed jobs. Not sure whether that will help, but let's try. I'm not aware of any "secret" button to accept failed tests.

@markelov208 markelov208 merged commit 5508e86 into master Nov 7, 2023
@markelov208 markelov208 deleted the geo/11740-add-thermal-constitutive-law branch November 7, 2023 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@avdg81 avdg81 avdg81 approved these changes

@rfaasse rfaasse rfaasse left review comments

Assignees

@markelov208 markelov208

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[GeoMechanicsApplication] Add thermal constitutive law
3 participants
@markelov208 @avdg81 @rfaasse