-
Notifications
You must be signed in to change notification settings - Fork 257
[GeoMechanicsApplication] Add point heat flux (source) condition for heat #12340
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.
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
Conversation
This is a trick, because winsows does not see difference in small or capital letters, but the compiliation fails on GCC. Delete them first then add them again with small letters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mohamed, very nice. Just a few comments.
applications/GeoMechanicsApplication/custom_conditions/thermal_point_flux_condition.cpp
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/custom_elements/transient_thermal_element.h
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/tests/test_transient_thermal.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Mohammed,
See the remarks.
Generally pull requests adress 1 thing, here the element and the condition get mixed.
That may complicate later searches.
Regards, Wijtze Pieter
applications/GeoMechanicsApplication/custom_conditions/thermal_point_flux_condition.cpp
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/custom_conditions/thermal_point_flux_condition.h
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/custom_elements/transient_thermal_element.h
Outdated
Show resolved
Hide resolved
...ment/test_thermal_heat_flux_line_element/test_thermal_point_flux_3D3N/ProjectParameters.json
Outdated
Show resolved
Hide resolved
...ment/test_thermal_heat_flux_line_element/test_thermal_point_flux_3D2N/ProjectParameters.json
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/custom_elements/transient_thermal_element.h
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/geo_mechanics_application.cpp
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/geo_mechanics_application_variables.cpp
Outdated
Show resolved
Hide resolved
...ts/test_thermal_element/test_thermal_heat_flux_line_element/Common/MaterialParameters2D.json
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/custom_elements/transient_thermal_element.h
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have a nice weekend after this hard work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very nice and concise PR with a good number of tests! I have some minor suggestions/questions, mainly I think it will be good to add documentation for the new tests.
applications/GeoMechanicsApplication/custom_conditions/thermal_point_flux_condition.cpp
Show resolved
Hide resolved
applications/GeoMechanicsApplication/custom_conditions/thermal_point_flux_condition.h
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/custom_elements/transient_thermal_element.h
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/custom_elements/transient_thermal_element.h
Outdated
Show resolved
Hide resolved
...ts/test_thermal_element/test_thermal_heat_flux_line_element/Common/MaterialParameters2D.json
Show resolved
Hide resolved
applications/GeoMechanicsApplication/geo_mechanics_application.cpp
Outdated
Show resolved
Hide resolved
# Conflicts: # applications/GeoMechanicsApplication/custom_elements/transient_thermal_element.h # applications/GeoMechanicsApplication/tests/test_thermal_element/test_thermal_line_element/Common/MaterialParameters2D.json # applications/GeoMechanicsApplication/tests/test_thermal_element/test_thermal_line_element/Common/MaterialParameters3D.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for implementing the review comments, I just have a few minor suggestions left.
...echanicsApplication/tests/test_thermal_element/test_thermal_heat_flux_line_element/README.md
Outdated
Show resolved
Hide resolved
...echanicsApplication/tests/test_thermal_element/test_thermal_heat_flux_line_element/README.md
Outdated
Show resolved
Hide resolved
test_name = 'test_thermal_heat_flux_2D3N' | ||
file_path = test_helper.get_file_path(os.path.join('test_thermal_element', 'test_thermal_heat_flux', test_name)) | ||
|
||
def check_temperature(self, test_directory, test_name, test_node, etalon_value): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me, from the name it's not directly clear that this function actually runs a simulation before checking results. Do you think we should reflect that in the name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, I do agree that the name of the test must reflects exactly what the test is meant for. Do you have suggestion for a more clear name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could consider to move the check itself back to the test bodies, and only run the thermal analysis here, which returns a list of temperatures. In the test body, all you need to do extra is get the temperature value at the relevant node (from the returned list of temperatures) and compare it to an expected value. That would reduce the parameter list here (since you no longer need test_node
and etalon_value
), and it would make it simpler to give this function a proper name, e.g. run_thermal_analysis
. Each test then consists of two lines: (1) running the thermal analysis, and (2) comparing the relevant temperature value. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A very nice, clean, and self-containing extension of your thermal analysis tools. I have several suggestions, but all of them are minor in nature. Well done, and thanks for keeping this PR relatively small!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please reformat this file using clang-format.
applications/GeoMechanicsApplication/custom_conditions/thermal_point_flux_condition.h
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/custom_conditions/thermal_point_flux_condition.h
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/custom_conditions/thermal_point_flux_condition.h
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/custom_conditions/thermal_point_flux_condition.h
Outdated
Show resolved
Hide resolved
...echanicsApplication/tests/test_thermal_element/test_thermal_heat_flux_line_element/README.md
Outdated
Show resolved
Hide resolved
...echanicsApplication/tests/test_thermal_element/test_thermal_heat_flux_line_element/README.md
Outdated
Show resolved
Hide resolved
...hermal_heat_flux_line_element/test_thermal_point_flux_2D2N/test_thermal_point_flux_2D2N.mdpa
Outdated
Show resolved
Hide resolved
test_name = 'test_thermal_heat_flux_2D3N' | ||
file_path = test_helper.get_file_path(os.path.join('test_thermal_element', 'test_thermal_heat_flux', test_name)) | ||
|
||
def check_temperature(self, test_directory, test_name, test_node, etalon_value): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we can combine test_directory
and test_name
in a single parameter (e.g. tail_of_test_path
), since the only thing you do with these two parameters is to combine them into a path.
test_name = 'test_thermal_heat_flux_2D3N' | ||
file_path = test_helper.get_file_path(os.path.join('test_thermal_element', 'test_thermal_heat_flux', test_name)) | ||
|
||
def check_temperature(self, test_directory, test_name, test_node, etalon_value): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could consider to move the check itself back to the test bodies, and only run the thermal analysis here, which returns a list of temperatures. In the test body, all you need to do extra is get the temperature value at the relevant node (from the returned list of temperatures) and compare it to an expected value. That would reduce the parameter list here (since you no longer need test_node
and etalon_value
), and it would make it simpler to give this function a proper name, e.g. run_thermal_analysis
. Each test then consists of two lines: (1) running the thermal analysis, and (2) comparing the relevant temperature value. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are very close. Only a remark about the text in the Readme file.
**Source files:** [Thermal line element with point flux condition](https://github.com/KratosMultiphysics/Kratos/tree/master/applications/GeoMechanicsApplication/tests/test_thermal_element/test_thermal_heat_flux_line_element) | ||
|
||
## Case Specification | ||
In this thermal test case, a 1 m height soil column is considered, where the initial temperature in the entire domain is set to 0 $\mathrm{[^\circ C]}$. The top boundary condition is set to be 0 $\mathrm{[^\circ C]}$. A heat flux of 100 $\mathrm{[W/m^2]}$ is set at the bottom boundary. The simulation spans 250 days to allow for a transition from an exponential to a linear temperature profile between the two sides. This test is conducted for various configurations, including 2D2N, 2D3N, 2D4N, 2D5N, 3D2N and 3D3N line elements. The temperature distribution along the depth is then evaluated with its own result. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand the remark: "from and exponential"
the initial situation is a constant zero temperature field.
**Source files:** [Thermal line element with point flux condition](https://github.com/KratosMultiphysics/Kratos/tree/master/applications/GeoMechanicsApplication/tests/test_thermal_element/test_thermal_heat_flux_line_element) | ||
|
||
## Case Specification | ||
In this thermal test case, a 1 m high soil column is considered, where the initial temperature in the entire domain is set to 0 $\mathrm{[^\circ C]}$. The top boundary condition is set to be 0 $\mathrm{[^\circ C]}$. A heat flux of 100 $\mathrm{[W/m^2]}$ is set at the bottom boundary. The simulation spans 250 days to allow for a transition from an exponential to a linear temperature profile between the two sides. This test is conducted for various configurations, including 2D2N, 2D3N, 2D4N, 2D5N, 3D2N and 3D3N line elements. The temperature distribution along the depth is then evaluated with its own result. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand the remark about exponential. It starts from a constant zero temperature field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to go.
📝 Description
As part of our ongoing development efforts, we need to implement a heat flux boundary condition for our 1D thermal line element in order to accurately simulate heat transfer behavior. Currently, the boundary conditions for this element type are limited to Dirichlet, and the inclusion of a heat flux boundary condition is crucial for modeling scenarios where heat transfer play a significant role. This `boundary is also crucial for wel element which will be implemented in the near future.
🆕 Changelog