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

Improve constructMeshFromGeometry #2459

Merged
merged 14 commits into from May 2, 2019

Conversation

@TomFischer
Copy link
Member

commented Apr 9, 2019

Some users (@Thomas-TK , @ErikNixdorf and Janine) need a more robust constructMeshesFromGeometry tool. This PR allows to give a larger search radius eps to the tool such that several mesh nodes are located in the eps-ball. Then the mesh node with the smallest distance to the given point is choosen.

  1. Feature description was added to the changelog
  2. Tests covering your feature were added?
  3. Any new feature or behavior change was documented?

@TomFischer TomFischer added the WIP 🏗 label Apr 9, 2019

@TomFischer TomFischer force-pushed the TomFischer:ImproveConstructMeshFromGeometry branch from 82b685e to 76d612d Apr 10, 2019

@codecov

This comment has been minimized.

Copy link

commented Apr 10, 2019

Codecov Report

Merging #2459 into master will increase coverage by 0.14%.
The diff coverage is 45.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2459      +/-   ##
==========================================
+ Coverage   32.57%   32.71%   +0.14%     
==========================================
  Files         554      554              
  Lines       20496    20505       +9     
  Branches     9656     9702      +46     
==========================================
+ Hits         6677     6709      +32     
+ Misses      10358    10271      -87     
- Partials     3461     3525      +64
Impacted Files Coverage Δ
MeshGeoToolsLib/BoundaryElementsAtPoint.h 100% <ø> (+100%) ⬆️
Applications/ApplicationsLib/ProjectData.cpp 0% <0%> (ø) ⬆️
MeshGeoToolsLib/MeshNodesOnPoint.h 100% <100%> (ø) ⬆️
MeshGeoToolsLib/MeshNodeSearcher.cpp 53.93% <26.31%> (+10.45%) ⬆️
MeshGeoToolsLib/BoundaryElementsSearcher.cpp 46.15% <33.33%> (+4.69%) ⬆️
MeshGeoToolsLib/ConstructMeshesFromGeometries.cpp 51.42% <33.33%> (+51.42%) ⬆️
MeshGeoToolsLib/MeshNodesOnPoint.cpp 33.33% <66.66%> (ø) ⬆️
MeshGeoToolsLib/BoundaryElementsAtPoint.cpp 62.96% <76.47%> (+62.96%) ⬆️
MathLib/Vector3.cpp 33.33% <0%> (-33.34%) ⬇️
MeshLib/CoordinateSystem.cpp 42.85% <0%> (-14.29%) ⬇️
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb28e59...1cf80c2. Read the comment docs.

{
if (&(*it)->getSurface() == &sfc)
if (&(mesh_nodes_along_surface->getSurface()) == &sfc)

This comment has been minimized.

Copy link
@endJunction

endJunction Apr 10, 2019

Member

Just a note. I'd think with operator== overloading for surfaces, polylines, and points, the three "equal" search functions would be equal.

@@ -25,6 +25,7 @@ BoundaryElementsAtPoint::BoundaryElementsAtPoint(
: _mesh(mesh), _point(point)
{
auto const node_ids = mshNodeSearcher.getMeshNodeIDs(_point);
unsigned min_dist_node_id = 0;

This comment has been minimized.

Copy link
@endJunction

endJunction Apr 10, 2019

Member

I'd think node ids are size_t...

@@ -34,17 +35,30 @@ BoundaryElementsAtPoint::BoundaryElementsAtPoint(
}
if (node_ids.size() > 1)
{
OGS_FATAL(
auto& mesh_nodes =

This comment has been minimized.

Copy link
@endJunction

endJunction Apr 10, 2019

Member

Structure like:

if (node_ids.empty()) OGS_FATAL();
if (node_ids.size() == 1) do_something_for_node(0);
if (!multiple_nodes_allowed) OGS_FATAL();
auto const nearest_node_id = std::min_element(...);
do_something_for_node(nearest_node_id);

This comment has been minimized.

Copy link
@endJunction

endJunction Apr 10, 2019

Member

the multiple_nodes_allowed must be an explicitly set command line argument.
I'd prefer, if we could stay strict by default.

Maybe it's done in next commit already...

#include "MeshLib/Mesh.h"
#include "MeshLib/MeshGenerators/MeshGenerator.h"

TEST(ConstructAdditionalMeshesFromGeoObjects, PointMesh)

This comment has been minimized.

Copy link
@endJunction

endJunction Apr 10, 2019

Member

Maybe an end-to-end test could cover the multiple nodes case; one failing when in default strict mode, another passing with multiple nodes allowed.

ERR("Empty element selection. Function createMeshFromElementsSelection "
"can not create mesh '%s'.",
mesh_name.c_str());
return nullptr;

This comment has been minimized.

Copy link
@TomFischer

TomFischer Apr 11, 2019

Author Member

The return nullptr causes several benchmarks to fail. In the current master sometimes meshes with zero nodes and zero elements are created automatically from geometries. I doubt these meshes are used later on for something.
@endJunction, @wenqing: Are there cases where such meshes are useful during a simulation? For discussion: Is a mesh with zero nodes and zero elements a valid mesh?

This comment has been minimized.

Copy link
@endJunction

endJunction Apr 11, 2019

Member

Zero nodes/elements mesh is a perfectly fine mesh, in my opinion, just a trivial one. Happens for partitioned meshes, for example, if boundary geometry doesn't have an intersection with that partition, but those result in .bin files. There should be no trivial meshes for serial cases used as input.
I've to admit, that I didn't look into the failing benchmarks (yet).

@TomFischer TomFischer force-pushed the TomFischer:ImproveConstructMeshFromGeometry branch 5 times, most recently from 62f2ee2 to 0251c6f Apr 11, 2019

@TomFischer TomFischer force-pushed the TomFischer:ImproveConstructMeshFromGeometry branch 2 times, most recently from e88b854 to 0d834d0 Apr 25, 2019

@TomFischer TomFischer removed the WIP 🏗 label Apr 30, 2019

"",
"multiple-nodes-allowed",
"allows that multiple mesh nodes are contained in the eps environment",
false, // optional argument

This comment has been minimized.

Copy link
@endJunction

endJunction Apr 30, 2019

Member
        false, // required argument
auto pnts = std::make_unique<std::vector<GeoLib::Point*>>();
auto pnt_name_map = std::make_unique<std::map<std::string, std::size_t>>();

for (std::size_t k(0); k < pnts_per_edge; k++) {

This comment has been minimized.

Copy link
@endJunction

endJunction Apr 30, 2019

Member

clang-format missing....

std::string pnt_name(
name + "-" + std::to_string(i) + "-" + std::to_string(j) + "-"
+ std::to_string(k));
pnt_name_map->insert(std::make_pair(pnt_name, id));

This comment has been minimized.

Copy link
@endJunction

endJunction Apr 30, 2019

Member

emplace without make_pair

@@ -52,11 +29,11 @@ TEST(GeoLib, GEOObjectsMergePoints)
// *** insert set of points number 0
GeoLib::Point shift (0.0,0.0,0.0);
names.emplace_back("PointSet0");
createSetOfTestPointsAndAssociatedNames(geo_objs, names[0], shift);
createSetOfTestPointsAndAssociatedNames(geo_objs, names[0], 8, shift);

This comment has been minimized.

Copy link
@endJunction

endJunction Apr 30, 2019

Member
int const points_per_edge = 8;
create...(geo_objs, names[0], points_per_edge, shift);
GeoLib::Point shift(0.0, 0.0, 0.0);
std::string geometry_name("AllMeshNodes");
// all points have a name
createSetOfTestPointsAndAssociatedNames(geometries, geometry_name, 10,

This comment has been minimized.

Copy link
@endJunction

endJunction Apr 30, 2019

Member

Same here, move 10 up to lines 25ff, and give it a name.

@TomFischer TomFischer force-pushed the TomFischer:ImproveConstructMeshFromGeometry branch from 97d7d91 to 5a71bf9 Apr 30, 2019

TomFischer added some commits Apr 9, 2019

[MGTL] Use nearest node out of nodes in eps environment.
This makes tools like constructMeshesFromGeometry more robust.
[T/GL] Mv createSetOfTestPoints... to seperate file.
The functionality can be used from other tests, too.
[MGTL] Add flag to make search routines flexible.
Makes the search for mesh nodes flexible. If tools, for instance ConstructMeshFromGeometry,
give a search radius such that more than one node is contained in the eps environment the
nearest node is returned.

@TomFischer TomFischer force-pushed the TomFischer:ImproveConstructMeshFromGeometry branch from 5a71bf9 to 78d3d7d May 2, 2019

@endJunction

This comment has been minimized.

Copy link
Member

commented May 2, 2019

@TomFischer TomFischer merged commit f5e8fa8 into ufz:master May 2, 2019

6 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
codecov/patch 45.16% of diff hit (target 32.57%)
Details
codecov/project 32.71% (+0.14%) compared to eb28e59
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
deploy/netlify Deploy preview ready!
Details
ufz.ogs #20190502.2 succeeded
Details

@TomFischer TomFischer deleted the TomFischer:ImproveConstructMeshFromGeometry branch May 2, 2019

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