-
Notifications
You must be signed in to change notification settings - Fork 239
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
First version of constraint boundary conditions #2145
Conversation
4fbed0f
to
6210aeb
Compare
ProcessLib::createLocalAssemblers< | ||
ConstraintDirichletBoundaryConditionLocalAssembler>( | ||
_bulk_mesh.getDimension(), _bc_mesh.getElements(), | ||
*_dof_table_boundary, 1, _local_assemblers, false, _integration_order, |
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.
How about to introduce two constants as:
const int component = 1;
const bool lower = false;
to replace the numbers, 1. false, in the argument list?
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.
Replacing the number makes the code more readable. Are you sure the suggested names are correct? What do think about the following replacements:
const int shape_function_order = 1;
const bool is_axisymmetric = false;
?
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.
Sorry I've checked a wrong argument list. The names of shape_function_order
and is_axisymmetric
are correct.
unsigned const number_nodes = boundary_element->getNumberOfNodes(); | ||
for (unsigned i = 0; i < number_nodes; ++i) | ||
{ | ||
unsigned const id = boundary_element->getNode(i)->getID(); |
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.
getID() returns std::size_t.
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.
Changed to auto
.
class ConstraintDirichletBoundaryCondition final : public BoundaryCondition | ||
{ | ||
public: | ||
/// @param parameter used for setting the values for the boundary condition |
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.
It is better to capitalize the first letter of argument comment.
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.
Capitalized.
} | ||
|
||
// make the pairs (node id, value) unique | ||
std::sort(tmp_bc_values.begin(), tmp_bc_values.end(), |
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.
The comment is about making vector unique, but here it's only sorted... ???
/// @param integration_order The order of the integration. | ||
ConstraintDirichletBoundaryConditionLocalAssembler( | ||
MeshLib::Element const& surface_element, | ||
std::size_t /*const local_matrix_size*/, |
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.
Commenting 'const' changes function's signature...
|
||
IntegrationMethod const _integration_method; | ||
std::size_t _bulk_element_id; | ||
unsigned _bulk_face_id; |
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.
these _bulk_*_id
are not const?
}); | ||
|
||
const int shape_function_order = 1; | ||
const bool is_axisymmetric = false; |
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.
isn't this a property of the bc_mesh?
// check if the boundary element is active | ||
if (_lower) | ||
{ | ||
if (_flux_values[boundary_element->getID()] < _constraint_threshold) |
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 comparator before the for-loop would be better:
auto is_flux = [&](std::size_t const element_id) {
return _lower ? _flux_values[element_id] < _constraint_threshold
: _flux_values[element_id] > _constraint_threshold; }
for ( ... boundary elements ...)
{
if (is_flux(boundary_element->getID())
{
continue;
}
...
}
//! \ogs_file_param{prj__process_variables__process_variable__boundary_conditions__boundary_condition__ConstraintDirichletBoundaryCondition__constraining_process_variable} | ||
config.getConfigParameter<std::string>("constraining_process_variable"); | ||
|
||
const int process_id = 0; // assume monolithic implementation |
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.
Maybe this can be checked if the constraining_process
is monolithic?
d71e80d
to
7db4f7b
Compare
private: | ||
MeshLib::Element const& _surface_element; | ||
|
||
std::vector<double> _detJ_times_integralMeasure; |
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.
integration point weight could also be added (multiplied) in the same scalar...
// for correct results it is necessary to multiply the normal with -1 | ||
surface_element_normal *= -1; | ||
|
||
std::size_t const n_integration_points = |
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'd recommend to use 'auto' here... The return type might change, and is currently an 'unsigned int'.
ProcessLib/HT/Tests.cmake
Outdated
ConstViscosityThermalConvection_pcs_0_ts_3_t_0.001010.vtu ConstViscosityThermalConvection_pcs_0_ts_3_t_0.001010.vtu p p 1e-15 1e-14 | ||
ConstViscosityThermalConvection_pcs_0_ts_4_t_0.101010.vtu ConstViscosityThermalConvection_pcs_0_ts_4_t_0.101010.vtu p p 1e-15 1e-14 | ||
ConstViscosityThermalConvection_pcs_0_ts_5_t_1.101010.vtu ConstViscosityThermalConvection_pcs_0_ts_5_t_1.101010.vtu p p 1e-15 1e-14 | ||
ConstViscosityThermalConvection_pcs_0_ts_6_t_10.000000.vtu ConstViscosityThermalConvection_pcs_0_ts_6_t_10.000000.vtu p p 1e-15 1e-14 |
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.
Could be reduced to two lines using GLOBs. See https://github.com/ufz/ogs/blob/master/ProcessLib/RichardsComponentTransport/Tests.cmake#L10-L11
|
||
// make the pairs (node id, value) unique | ||
// first: sort the (node id, value) pairs according to the node id | ||
std::sort(tmp_bc_values.begin(), tmp_bc_values.end(), |
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.
Sorry, but I still don't see where the tmp_bc_values are "made unique". There is a copy, but not a std::unique followed by erase....
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 changed the comment.
GlobalVector const&)> | ||
getFlux); | ||
|
||
~ConstraintDirichletBoundaryCondition() = default; |
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.
Can be dropped, isn't it?
// At the moment (2018-04-26) the surface normal is not oriented | ||
// according to the right hand rule | ||
// for correct results it is necessary to multiply the normal with -1 | ||
surface_element_normal *= -1; |
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.
Computing a normalized surface normal for element/face could be a free function in MeshLib.
Eigen::Map<Eigen::RowVectorXd const>(bulk_flux.data(), | ||
bulk_flux.size()) | ||
.dot(Eigen::Map<Eigen::RowVectorXd const>( | ||
surface_element_normal.getCoords(), 3))); |
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.
Since the surface element normal does not change, it could be stored in the integration point data...
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.
The normal doesn't change for all integration points, am I right? For this reason, I made the normal an attribute of the object and did not move it to the integration point data.
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.
The current implementation is returning a constant normal vector for each element. This is not correct...
7db4f7b
to
ac1a2b0
Compare
First version of constraint boundary conditions
OpenGeoSys development has been moved to GitLab. |
For discussion: