-
Notifications
You must be signed in to change notification settings - Fork 248
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
[GeoMechanicsApplication] Add 1D functionality to existing 2D/3D Pw element #12292
Conversation
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 Mohamed, a great work and many tests added. I put some comments. It looks that we have already implemented similar things.
applications/GeoMechanicsApplication/custom_elements/transient_Pw_line_element.h
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/custom_elements/transient_Pw_line_element.cpp
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/custom_elements/transient_Pw_line_element.h
Show resolved
Hide resolved
applications/GeoMechanicsApplication/custom_elements/transient_Pw_line_element.h
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/custom_elements/transient_Pw_line_element.h
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/custom_elements/transient_Pw_line_element.h
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/custom_elements/transient_Pw_line_element.h
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/custom_elements/transient_Pw_line_element.h
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/custom_elements/transient_Pw_line_element.h
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/tests/test_pressure_line_element.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.
This is nicely added as derived from the Element
class and does not add to the hierarchy tree we are trying to clean up!
There are still some duplications in here that I think we should be able to clean up. We can have a discussion what would be the best way to do this (we could for example create a small PR to extract some functionality to a utility function, such that we can re-use it here and in the other places where we have defined some of these functions.
applications/GeoMechanicsApplication/custom_elements/transient_Pw_line_element.cpp
Show resolved
Hide resolved
applications/GeoMechanicsApplication/custom_utilities/element_utilities.hpp
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/custom_elements/transient_Pw_line_element.h
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/custom_elements/transient_Pw_line_element.h
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/custom_elements/transient_Pw_line_element.h
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/tests/test_pressure_line_element.py
Show resolved
Hide resolved
applications/GeoMechanicsApplication/custom_elements/transient_Pw_line_element.h
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/custom_elements/transient_Pw_line_element.h
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/custom_elements/transient_Pw_line_element.h
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/custom_elements/transient_Pw_line_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.
Hi Mohamed, many thanks for the conversation and fixing the code. Let's move on ;)
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.
Dear Mohammed,
Not sure if all my remarks on the code should be taken up. However, I find it quite important to have follow up on some of the remarks on tests. 3D is unintentionally using 2D elements.
Regards, Wijtze Pieter
applications/GeoMechanicsApplication/custom_elements/transient_Pw_line_element.h
Outdated
Show resolved
Hide resolved
const BoundedMatrix<double, TNumNodes, TNumNodes>& rCompressibilityMatrix, | ||
double DtPressureCoefficient) | ||
{ | ||
rLeftHandSideMatrix = rPermeabilityMatrix + DtPressureCoefficient * rCompressibilityMatrix; |
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.
Very readable, like a formula.
The name of the function however says AddContributionsTo. That is not what this formula states.
array_1d<double, TNumNodes>{-prod(rCompressibilityMatrix, GetNodalValuesOf(DT_WATER_PRESSURE))}; | ||
const auto permeability_vector = | ||
array_1d<double, TNumNodes>{-prod(rPermeabilityMatrix, GetNodalValuesOf(WATER_PRESSURE))}; | ||
rRightHandSideVector = compressibility_vector + permeability_vector + rFluidBodyVector; |
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.
Same here: very readable like a formula.
The function name AddContributionsTo sounds like += i.s.o. the =
applications/GeoMechanicsApplication/custom_elements/transient_Pw_line_element.h
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/custom_elements/transient_Pw_line_element.h
Outdated
Show resolved
Hide resolved
...cation/tests/test_pressure_line_element/test_oblique_line_element3D2N/ProjectParameters.json
Outdated
Show resolved
Hide resolved
...ation/tests/test_pressure_line_element/test_vertical_line_element3D2N/ProjectParameters.json
Outdated
Show resolved
Hide resolved
...est_pressure_line_element/test_vertical_line_element3D2N/test_vertical_line_element3D2N.mdpa
Outdated
Show resolved
Hide resolved
...ation/tests/test_pressure_line_element/test_vertical_line_element3D3N/ProjectParameters.json
Outdated
Show resolved
Hide resolved
...est_pressure_line_element/test_vertical_line_element3D3N/test_vertical_line_element3D3N.mdpa
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.
Thanks a lot for all your efforts in processing review comments and adding the documentation, I have one question left about the tangential vector, but next to the open comments from Wijtze Pieter, I have nothing to add!
applications/GeoMechanicsApplication/custom_elements/transient_Pw_line_element.h
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/tests/test_pressure_line_element/README.md
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/tests/test_pressure_line_element.py
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.
Looks good to me!
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.
Lets go and in a separate commit make naming for LHS and RHS computation consistent in both Pw and T elements.
"BIOT_COEFFICIENT" : 1.000000e+00, | ||
"RETENTION_LAW" : "SaturatedLaw", | ||
"SATURATED_SATURATION" : 1.000000e+00, | ||
"RESIDUAL_SATURATION" : 0.000000e+00 |
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.
Is RESIDUAL_SATURATION needed for the SaturatedLaw? The 3D materials parameters files shows that it is possible without.
📝 Description
As user we would like to implement well-element for geothermal high capacity heat storage. For this, we need 1D pressure and thermal elements. However, 1D pressure element does not exist in the current implementation.
Here, we would like to add a generic 1D pressure line element (to be used for well-element in the later implementation).
🆕 Changelog