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

Remove MeshSubsets #2135

Merged
merged 13 commits into from Jun 8, 2018

Conversation

Projects
None yet
3 participants
@endJunction
Copy link
Member

endJunction commented May 31, 2018

This is a preview, there is still a small memory leak in this (commented), but the remaining parts will stay as they are now. Memory leaks fixed.

A never-used feature: a variable defined on different meshes.
While removing the MeshSubsets, the MeshSubset class was modified too with notable changes:

  • No memory management of the vectors. Now the node vectors are managed solely by the caller/creator of the MeshSubset.
  • Subsetting via elements is removed, since it was not used anywhere.

The first part deals with the MeshSubsets removal, while the second part fixes a memory leak and updates the MeshSubset.

Preparation to named meshes created from geometries. The creation happens now in BCs (Neumann).

@endJunction endJunction force-pushed the endJunction:RemoveMeshSubsets branch 2 times, most recently from 8334bce to a7406b5 Jun 2, 2018

@endJunction endJunction removed the WIP 🏗 label Jun 2, 2018

@endJunction endJunction force-pushed the endJunction:RemoveMeshSubsets branch from a7406b5 to f3e6f79 Jun 2, 2018

@endJunction

This comment has been minimized.

Copy link
Member Author

endJunction commented Jun 2, 2018

The failing jenkins/mac job is due to missing files on the website: #2136.

@endJunction endJunction requested a review from chleh Jun 2, 2018

@endJunction endJunction force-pushed the endJunction:RemoveMeshSubsets branch 2 times, most recently from a2fc618 to 4b685cf Jun 4, 2018

@chleh

chleh approved these changes Jun 4, 2018

Copy link
Collaborator

chleh left a comment

Mostly minor things. Only one thing to discuss:

Wouldn't it be better to store the nodes set as a shared_ptr inside the subset instead of storing it in the caller?

// For all MeshSubsets and each of their MeshSubset's and each element
// of that MeshSubset save a line of global indices.
// For all MeshSubset's and each element of that MeshSubset save a line of
// global indices.

This comment has been minimized.

@chleh

chleh Jun 4, 2018

Collaborator

Maybe better:
For each element of each MeshSubset save a line...

// For all MeshSubsets and each of their MeshSubset's and each element
// of that MeshSubset save a line of global indices.
// For all MeshSubset's and each element of that MeshSubset save a line of
// global indices.

This comment has been minimized.

@chleh

chleh Jun 4, 2018

Collaborator

same as above.

@@ -29,110 +29,96 @@ GlobalIndexType const MeshComponentMap::nop =

#ifdef USE_PETSC
MeshComponentMap::MeshComponentMap(
const std::vector<MeshLib::MeshSubsets>& components, ComponentOrder order)
std::vector<MeshLib::MeshSubset> const& mesh_subsets, ComponentOrder order)

This comment has been minimized.

@chleh

chleh Jun 4, 2018

Collaborator

Minor: maybe keep the old name components since they symbolize variable components?

comp_id++;
}
}
#else
MeshComponentMap::MeshComponentMap(
const std::vector<MeshLib::MeshSubsets>& components, ComponentOrder order)
std::vector<MeshLib::MeshSubset> const& mesh_subsets, ComponentOrder order)

This comment has been minimized.

@chleh

chleh Jun 4, 2018

Collaborator

likewise.

@@ -25,15 +25,14 @@ CalculateSurfaceFlux::CalculateSurfaceFlux(
_local_assemblers.resize(boundary_mesh.getElements().size());

// needed to create dof table
auto mesh_subset_all_nodes = std::make_unique<MeshLib::MeshSubset const>(
auto mesh_subset_all_nodes = std::make_unique<MeshLib::MeshSubset>(

This comment has been minimized.

@chleh

chleh Jun 4, 2018

Collaborator

No need for a unique_ptr here, I assume.

@@ -251,7 +251,7 @@ class Process

protected:
MeshLib::Mesh& _mesh;
std::unique_ptr<MeshLib::MeshSubset const> _mesh_subset_all_nodes;
std::unique_ptr<MeshLib::MeshSubset> _mesh_subset_all_nodes;

This comment has been minimized.

@chleh

chleh Jun 4, 2018

Collaborator

not const anymore?

@@ -46,7 +46,7 @@ class GenericNonuniformNaturalBoundaryCondition final : public BoundaryCondition

std::unique_ptr<MeshLib::Mesh> _boundary_mesh;

std::unique_ptr<MeshLib::MeshSubset const> _mesh_subset_all_nodes;
std::unique_ptr<MeshLib::MeshSubset> _mesh_subset_all_nodes;

This comment has been minimized.

@chleh

chleh Jun 4, 2018

Collaborator

Minor: Is the constness change necessary?

@@ -57,7 +57,7 @@ class NormalTractionBoundaryCondition final : public BoundaryCondition
/// defined.
std::vector<MeshLib::Element*> _elements;

std::unique_ptr<MeshLib::MeshSubset const> _mesh_subset_all_nodes;
std::unique_ptr<MeshLib::MeshSubset> _mesh_subset_all_nodes;

This comment has been minimized.

@chleh

chleh Jun 4, 2018

Collaborator

likewise

MeshLib::MeshSubset bc_mesh_subset =
mesh_subset.getIntersectionByNodes(nodes);
_nodes_subset = nodesNodesIntersection(mesh_subset.getNodes(), nodes);
MeshLib::MeshSubset bc_mesh_subset(mesh_subset.getMesh(), &_nodes_subset);

This comment has been minimized.

@chleh

chleh Jun 4, 2018

Collaborator

Wouldn't it be better to store the nodes set as a shared_ptr inside the subset instead of stroing it in the caller?

@endJunction

This comment has been minimized.

Copy link
Member Author

endJunction commented Jun 4, 2018

@chleh Thank you for the fast review. I'll try to add back the constness and update the comments.

The nodes vector is usually the same as used by the mesh, and only a const ref is stored in the mesh subset. The only different story is the node vector for bcs, but with introduction of boundary meshes, it will also be the same node vector as used by the (boundary) mesh.
LIE is the only special case.

@wenqing

wenqing approved these changes Jun 5, 2018

Copy link
Member

wenqing left a comment

Looks good. Only one suggestion to change the name of MeshSubset to MeshNodeSubset

@@ -141,15 +143,18 @@ class NumLibLocalToGlobalIndexMapMultiDOFTest : public ::testing::Test
compute_global_index);

std::unique_ptr<const MeshLib::Mesh> mesh;
std::unique_ptr<const MeL::MeshSubset> mesh_items_all_nodes;
std::unique_ptr<MeL::MeshSubset> mesh_items_all_nodes;

This comment has been minimized.

@wenqing

wenqing Jun 5, 2018

Member

You may have the same reason as that in above to remove const


GeoLib::GEOObjects geo_objs;

std::unique_ptr<NL::LocalToGlobalIndexMap> dof_map;
std::unique_ptr<NL::LocalToGlobalIndexMap> dof_map_boundary;

std::unique_ptr<MeL::MeshSubset const> mesh_items_boundary;
std::unique_ptr<MeL::MeshSubset> mesh_items_boundary;

This comment has been minimized.

@wenqing

wenqing Jun 5, 2018

Member

Here the same

return active_nodes;
}

/// A subset of nodes on a single mesh.

This comment has been minimized.

@wenqing

wenqing Jun 5, 2018

Member

We may use elements as a MeshSubset to perform deactivate/activating elements for different processes in a coupling or for excavation modelling. If only nodes are kept in this class, the class name, I think, it can be termed as MeshNodeSubset. Then if we need the functionality of deactivate/activating elements, we can introduce a class of MeshElementSubset.

This comment has been minimized.

@endJunction

endJunction Jun 6, 2018

Author Member

I agree with renaming in general. I'd postpone it until we have the hypothetical MeshElementSubset because the rename touches 43 files.

@chleh

This comment has been minimized.

Copy link
Collaborator

chleh commented Jun 7, 2018

Thanks for the changes and the comment on the nodes vector.

@endJunction endJunction force-pushed the endJunction:RemoveMeshSubsets branch from 1864619 to bb381c5 Jun 8, 2018

endJunction added some commits May 31, 2018

[MeL] MS: Extract nodesNodesIntersection.
The function is independent of the MeshSubset.
[PL] BC: Replace getIntersectionByNodes.
The creation of a mesh subset via intersection is
split into two functions; the first one handles the
nodes intersection (and the resulting memory is managed
by the current scope), and in the second step the
actual MeshSubset object is created, w/o any memory
management obligations.
Add missing <numeric> include.
DOF: partial_sum
BC: iota

@endJunction endJunction force-pushed the endJunction:RemoveMeshSubsets branch from bb381c5 to 74adba4 Jun 8, 2018

@endJunction endJunction merged commit b799704 into ufz:master Jun 8, 2018

2 of 3 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
deploy/netlify Deploy preview ready!
Details

@endJunction endJunction deleted the endJunction:RemoveMeshSubsets branch Jun 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.