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

Move BC meshes; preparations #2140

Merged
merged 6 commits into from Jun 12, 2018

Conversation

Projects
None yet
3 participants
@endJunction
Copy link
Member

endJunction commented Jun 8, 2018

A set of basic changes required for the actual "Move BC meshes #2141" PR.

endJunction added some commits May 27, 2018

[MeL] getEdge and getFace use bulk elements id.
Before the id was (by default) set to std::size_t(-1).
[MeL] Implement cloneElements. Use in BCs.
The cloneElements is needed outside of MeshGeoToolsLib.

@endJunction endJunction referenced this pull request Jun 9, 2018

Merged

Move bc meshes #2141

@endJunction endJunction force-pushed the endJunction:MoveBCMeshesPreparations branch from 2788815 to 1f3d270 Jun 9, 2018

@chleh
Copy link
Collaborator

chleh left a comment

Essentially only a performance-related comment.

// Special treatment of the non-line elements because the identifyFace
// implementation expects three valid node pointers in the call. We can
// guarantee only one valid node, and calling the identifyFace would
// most likely lead to a SEGV.

This comment has been minimized.

@chleh

chleh Jun 11, 2018

Collaborator

Maybe I don't understand it right, but isn't that necessary only for points then?

This comment has been minimized.

@endJunction

endJunction Jun 11, 2018

Author Member

It is for points only (see function name: boundaryElementsAtPoint), but maybe I missed you point ;)

Note to myself: a similar case happens when searching for lines (having two points) in an element's face... but the case didn't happen yet.

This comment has been minimized.

@endJunction

endJunction Jun 12, 2018

Author Member

if there is something more to discuss, I'll prepare another PR, but for now we have to move on...

// testing if the given nodes belong to the mesh.
auto node_is_part_of_mesh =
[mesh_nodes = _msh.getNodes()](MeshLib::Node* const& n) {
auto it = std::find(begin(mesh_nodes), end(mesh_nodes), n);

This comment has been minimized.

@chleh

chleh Jun 11, 2018

Collaborator

This is a pointer comparison. Is that correct?

This comment has been minimized.

@endJunction

endJunction Jun 11, 2018

Author Member

Yes. Just a logical check (not geometrical) if the subset of nodes is indeed a part of the mesh nodes vector.

}
return true;
};
if (!std::all_of(begin(_nodes), end(_nodes), node_is_part_of_mesh))

This comment has been minimized.

@chleh

chleh Jun 11, 2018

Collaborator

The runtime complexity of that is length(mesh_nodes) * length(mesh_subset) isn't that terribly slow?

This comment has been minimized.

@endJunction

endJunction Jun 11, 2018

Author Member

You are right, although it is not that slow that it would have drawn my attention to it.
I'll update for more optimized version in this PR. The special case (also very fast to check) is the whole mesh subset, where the nodes are the same as the mesh's nodes.

This comment has been minimized.

@endJunction

endJunction Jun 11, 2018

Author Member

Improving it, tested with GWF/square_1e5_neumann.prj

ufz/master time is 2.88s
the unsorted vector naive implementation runs in 4.95s
the sorted vector with binary search runs in 2.90s
and checking for the vector addresses before expensive search is indistinguishable from the ufz/master.

Taking the last one :)

auto it = std::find(begin(mesh_nodes), end(mesh_nodes), n);
if (it == end(mesh_nodes))
{
ERR("A node %d in mesh subset is not a part of the mesh.",

This comment has been minimized.

@TomFischer

TomFischer Jun 11, 2018

Member

Maybe outputting the coordinates too will help to find faster a possible reason for the error?

@TomFischer
Copy link
Member

TomFischer left a comment

endJunction added some commits May 26, 2018

[MGTL] BoundaryElemAtPoint checks for line element
The identifyFace call with only one valid point in the
nodes list is only valid for lines. Calling this
function on other elements will most likely end in
a SEGV.

The problem didn't appear before, because this function
was only called for line elements.

@endJunction endJunction force-pushed the endJunction:MoveBCMeshesPreparations branch from e48471e to 060afc8 Jun 11, 2018

@endJunction endJunction merged commit 213acdd into ufz:master Jun 12, 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:MoveBCMeshesPreparations branch Jun 12, 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.