Skip to content
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

Allow multiple meshes in project #2156

Merged
merged 30 commits into from
Aug 23, 2018
Merged

Allow multiple meshes in project #2156

merged 30 commits into from
Aug 23, 2018

Conversation

endJunction
Copy link
Member

follow up of ctest fixes #2155

Main features are:

  1. the addition of
   <meshes>
      <mesh>a.vtu</mesh>
      ...
   </meshes>

possibility (instead of a single mesh and geometry) to the project file.

  1. Using the mesh names in non-uniform boundary conditions.

@endJunction
Copy link
Member Author

I'm a little stuck with the parallelization part here. As soon as this is available, we can continue on the review...

Copy link
Collaborator

@chleh chleh left a comment

Choose a reason for hiding this comment

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

So far it looks fine. Only minor comments.

auto optional_meshes = config.getConfigSubtreeOptional("meshes");
if (optional_meshes)
{
DBUG("OK Reading multiple meshes.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe drop OK.

if (optional_meshes)
{
DBUG("OK Reading multiple meshes.");
for (auto mesh_config_line :
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe only mesh_config.

{ // Read single mesh with geometry.
WARN(
"Consider switching from mesh and geometry input to multiple "
"meshes input.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here, pointing to some docu would be nice. TBD later? Add TODO?

auto pv =
ProcessLib::ProcessVariable{var_config, _mesh_vec, _parameters};
// Either the mesh name is given, or the first mesh's name will be
// taken. Taking the first mesh's value is depricated.
Copy link
Collaborator

Choose a reason for hiding this comment

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

deprecated

ProcessLib::ProcessVariable{var_config, _mesh_vec, _parameters};
// Either the mesh name is given, or the first mesh's name will be
// taken. Taking the first mesh's value is depricated.
std::string const mesh_name =
Copy link
Collaborator

Choose a reason for hiding this comment

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

There were times when you used to use auto much more often. ^^

@@ -30,6 +30,12 @@ MeshLib::Mesh const& findMeshInConfig(
//
std::string mesh_name; // Either given directly in <mesh> or constructed
// from <geometrical_set>_<geometry>.

#ifdef DOCUMENTATION
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe rename the macro, e.g., OGS_THIS_IS_DOXYGEN_RUN.

example in LIE, where the displacement jump variable is defined only on the
fractures, or in case of hydro mechanics, where the pressure variable is defined
on lower order elements, the boundary mesh is not strictly required to coincide
with the part of the process variable's boundary part.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually it's not checked if they coincide, but only if they have the same number of nodes, is it?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is assumed that geometrically the boundary mesh is fitting the bulk mesh. This is not checked, but "guaranteed" if the constructMeshesFromGeometries tool is used.
Now, if the strict_bc_domain is set, then all nodes of the bc_mesh for that variable must have a valid entry in the DOF table.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's what I wanted to say. 😄

And I'd add the following to the documentation:

It is assumed that geometrically the boundary mesh is fitting the bulk mesh.

@@ -75,9 +75,17 @@ createNonuniformNeumannBoundaryCondition(
mapping_to_bulk_nodes_property.c_str());
}

auto const strict_bc_domain =
Copy link
Collaborator

Choose a reason for hiding this comment

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

can't that be moved to a central place for all BCs?

@@ -2023,7 +2023,7 @@ INCLUDE_FILE_PATTERNS =
# recursively expanded use the := operator instead of the = operator.
# This tag requires that the tag ENABLE_PREPROCESSING is set to YES.

PREDEFINED =
PREDEFINED = DOCUMENTATION
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe rename the macro

@@ -86,6 +86,21 @@
</xs:complexType>
</xs:element>
<xs:element name="geometry" type="xs:string" minOccurs="0"/>
<xs:element name="meshes" minOccurs="0">
Copy link
Collaborator

Choose a reason for hiding this comment

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

maxOccurs="1" or is that the default?

@endJunction endJunction mentioned this pull request Jul 13, 2018
Copy link
Member

@wenqing wenqing left a comment

Choose a reason for hiding this comment

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

Looks good. Only small stuffs.

@@ -0,0 +1 @@
\ogs_missing_documentation
Copy link
Member

Choose a reason for hiding this comment

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

missing documentation.

/// The \c strict_bc_domain flag is controlling if the geometrical boundary
/// domain and the domain of the global components (varaible_id,
/// component_ids) must coincide. If set to false the not found global
/// indices will not inserted into the derived DOF table.
Copy link
Member

Choose a reason for hiding this comment

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

will not be

config.getConfigParameter<bool>("strict_bc_domain", true);
if (!strict_bc_domain)
{
DBUG("Allow non-strict bc domain and global component mapping.");
Copy link
Member

Choose a reason for hiding this comment

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

bc --> BC?

@@ -45,6 +39,23 @@ GenericNaturalBoundaryCondition<BoundaryConditionData,
dof_table_bulk.getNumberOfVariableComponents(variable_id));
}

if (_bc_mesh.getDimension() + 1 != global_dim)
Copy link
Member

Choose a reason for hiding this comment

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

How about the case of a 3D mesh getting 1D BC?

Copy link
Collaborator

Choose a reason for hiding this comment

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

With out "ordinary" natural BCs a 1D BC for a 3D domain doesn't make sense, does it? For something like line sources the case is different, of course. But they don't fit our natural BCs anyway.

Copy link
Member

Choose a reason for hiding this comment

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

I see that BCs on lines for 3D problem are excluded from this type of definition.

}

if (_boundary_mesh->getDimension() + 1 != global_dim)
if (_bc_mesh.getDimension() + 1 != global_dim)
Copy link
Member

Choose a reason for hiding this comment

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

The same question.

config.getConfigParameter<bool>("strict_bc_domain", true);
if (!strict_bc_domain)
{
DBUG("Allow non-strict bc domain and global component mapping.");
Copy link
Member

Choose a reason for hiding this comment

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

bc-->BC?


if (!boundary_mesh)
// Surface mesh and bulk mesh must have equal axial symmetry flags!
if (boundary_mesh.isAxiallySymmetric() != bulk_mesh.isAxiallySymmetric())
Copy link
Member

Choose a reason for hiding this comment

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

I saw this twice. Is it possible to set it as a function?

n_and_weight.N, flux);
_local_rhs.noalias() +=
n_and_weight.N * (n_and_weight.weight * flux);
// pos.setIntegrationPoint(ip);
Copy link
Member

Choose a reason for hiding this comment

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

Is the comment needed?

Removing most of the code duplication in the BCs/STs creation.
Rename bulk_node_ids and bulk_element_ids.
Use multiple meshes for setting the (heterogeneous) BCs.
This is needed to repeat parts of the code solely for the
documentation purposes.
Used in construction of mesh names in two places.
This unifies the name.
Copy link
Member

@wenqing wenqing left a comment

Choose a reason for hiding this comment

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

👍

@endJunction endJunction merged commit d9eed42 into ufz:master Aug 23, 2018
@endJunction endJunction deleted the AllowMultipleMeshesInProject_j branch August 23, 2018 16:17
@ogsbot
Copy link
Member

ogsbot commented Jun 19, 2020

OpenGeoSys development has been moved to GitLab.

See this pull request on GitLab.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants