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

BoundaryElementSearch: Return bulk element id and bulk element face id. #2125

Merged
merged 10 commits into from May 23, 2018

Conversation

Projects
None yet
3 participants
@TomFischer
Copy link
Member

TomFischer commented May 16, 2018

Preparation PR for the constraint boundary condition PR that will be created soon.

@chleh

chleh approved these changes May 17, 2018

Copy link
Collaborator

chleh left a comment

Looks good in general.

@@ -56,20 +56,28 @@ class BoundaryElementsSearcher
*/
std::vector<MeshLib::Element*> const& getBoundaryElements(GeoLib::GeoObject const& geoObj);

std::vector<std::pair<std::size_t, unsigned>> const& getBulkIDs(

This comment has been minimized.

@chleh

chleh May 17, 2018

Collaborator

Documentation?

/**
* generate boundary elements at the given point.
* @param point Search the mesh for given point
* @return a vector of boundary elements
*/
std::vector<MeshLib::Element*> const& getBoundaryElementsAtPoint(
GeoLib::Point const& point);
std::vector<std::pair<std::size_t, unsigned>> const& getBulkIDsAtPoint(

This comment has been minimized.

@chleh

chleh May 17, 2018

Collaborator

Documentation?

break;
default:
const static std::vector<std::pair<std::size_t, unsigned>> dummy;
return dummy;

This comment has been minimized.

@chleh

chleh May 17, 2018

Collaborator

Shouldn't that case rather be an error?

This comment has been minimized.

@TomFischer

TomFischer May 18, 2018

Author Member

In principle you are right. I made it consistent with the existing code. The new method getBulkIDs() has the same behaviour as the similar method getBoundaryElements(). I don't want to break the consistency in this PR. @chleh Is this okay for you?

This comment has been minimized.

@chleh

chleh May 18, 2018

Collaborator

OK.

std::array<MeshLib::Node*, 1> const nodes = {{
const_cast<MeshLib::Node*>(_mesh.getNode(node_ids[0]))}};

_boundary_elements.push_back(new MeshLib::Point{nodes, node_ids[0]});
for (auto const* bulk_element : _mesh.getNode(node_ids[0])->getElements())
{
_bulk_ids.emplace_back(std::make_pair(

This comment has been minimized.

@chleh

chleh May 17, 2018

Collaborator

Minor Note: emplace_back shouldn't require std::make_pair.

{
_bulk_ids.emplace_back(std::make_pair(
bulk_element->getID(),
bulk_element->identifyFace(mesh_nodes.data()+node_ids[0])));

This comment has been minimized.

@chleh

chleh May 17, 2018

Collaborator

(Not thir PR) That identifyFace does not have a const argument looks like a mistake.

});

// sort picked edges according to a distance of their first node along the
// polyline

This comment has been minimized.

@chleh

chleh May 17, 2018

Collaborator

Is that necessary to speed up some subsequent linear searches?

This comment has been minimized.

@TomFischer

TomFischer May 18, 2018

Author Member

The boundary elements have been sorted in the previous version of the implementation, too. I think the sort was necessary in OGS-5 for some reason. I'm not sure if it is needed anymore in OGS-6.

This comment has been minimized.

@chleh

chleh May 18, 2018

Collaborator

Probably currently it's too much work to check if that code is still needed. But could you please add a comment that the sorting might be unnecessary. Because otherewise somebody who will refactor that piece of code later one might be pretty puzzled.

std::find(node_ids_on_poly.begin(), node_ids_on_poly.end(), e2->getNodeIndex(0)));
return (dist1 < dist2);
});
// zip _boundary_elements and _bulk_element_ids

This comment has been minimized.

@endJunction

endJunction May 22, 2018

Member

Very similar algorithm zip-sort-unzip is used in BaseLib::quicksort. Maybe the BaseLib version can be generalized and used for this purpose? The only difference there is that begin is a pointer and offset, and same for the end.

@@ -60,6 +60,8 @@ class BoundaryElementsAlongPolyline
*/
std::vector<MeshLib::Element*> const& getBoundaryElements() const {return _boundary_elements; }

std::vector<std::pair<std::size_t, unsigned>> const& getBulkIDs() const;

This comment has been minimized.

@endJunction

endJunction May 22, 2018

Member

Short comment about what is the first and second pair's element meaning.

@@ -83,6 +85,8 @@ class BoundaryElementsAlongPolyline
MeshLib::Mesh const& _mesh;
GeoLib::Polyline const& _ply;
std::vector<MeshLib::Element*> _boundary_elements;

std::vector<std::pair<std::size_t, unsigned>> _bulk_ids;

This comment has been minimized.

@endJunction

endJunction May 22, 2018

Member

Same comment here.

@@ -37,10 +37,19 @@ BoundaryElementsAtPoint::BoundaryElementsAtPoint(
"one is expected.",
node_ids.size(), _point[0], _point[1], _point[2]);

auto& mesh_nodes =
const_cast<std::vector<MeshLib::Node*>&>(_mesh.getNodes());

This comment has been minimized.

@endJunction

endJunction May 22, 2018

Member

Not sure here: Maybe the const cast is not necessary? It is only used in the identifyFace call through a pointer...

This comment has been minimized.

@TomFischer

TomFischer May 23, 2018

Author Member

identifyFace expects a non const array. As @chleh mentioned this is probably bad design in identifyFace.

private:
MeshLib::Mesh const& _mesh;
GeoLib::Point const& _point;
std::vector<MeshLib::Element*> _boundary_elements;
std::vector<std::pair<std::size_t, unsigned>> _bulk_ids;

This comment has been minimized.

@endJunction

endJunction May 22, 2018

Member

same comment here etc.

for (auto const& boundary_elements : _boundary_elements_at_point)
{
if (boundary_elements->getPoint() == point)
return boundary_elements->getBulkIDs();

This comment has been minimized.

@endJunction

endJunction May 22, 2018

Member

The STL version:

auto it = std::find_if(begin(_boundary_elements_at_point), end(_boundary_elements_at_point,
   [&point](auto const& boundary_element)  {
      return boundary_elements->getPoint() == point;
   });
if (it != _boundary_elements_at_point.end())
    return it->getBulkIDs();

... is not necessarily shorter

This comment has been minimized.

@TomFischer

TomFischer May 23, 2018

Author Member

It is not shorter and not substantially more readable. I would like to keep my implementation. Is this okay for you?

/// @return A vector of id pairs will be returned where the first item of
/// the pair is the element id and the second item of the pair is the face
/// id.
std::vector<std::pair<std::size_t, unsigned>> const& getBulkIDs(

This comment has been minimized.

@endJunction

endJunction May 22, 2018

Member

Ah, I found the docu. You might want to use \copydoc if it's all the same text.

@endJunction
Copy link
Member

endJunction left a comment

Minor comments.

@endJunction

This comment has been minimized.

Copy link
Member

endJunction commented May 23, 2018

Hope the changes still give same results. If not, drop the BL changes and merge.

@TomFischer TomFischer force-pushed the TomFischer:NewFunctionalityInElementSearch branch from 1820992 to 96a7e1a May 23, 2018

@endJunction endJunction merged commit e6a6030 into ufz:master May 23, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details

@TomFischer TomFischer deleted the TomFischer:NewFunctionalityInElementSearch branch May 24, 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.