[SBM-IGA] Adding snake_sbm_utilities and test #13095
Conversation
Co-authored-by: Nicolò Antonelli <[email protected]>
|
-- Not ready -- |
…into PR_small_snakeSBMUtilities
|
@rubenzorrilla |
| // Main authors: Nicolo' Antonelli | ||
| // Main authors: Andrea Gorgi |
There was a problem hiding this comment.
| // Main authors: Nicolo' Antonelli | |
| // Main authors: Andrea Gorgi | |
| // Main authors: Nicolo' Antonelli | |
| // Andrea Gorgi |
There was a problem hiding this comment.
Amend the formatting issues as it is a bit annoying to go through it right now. Please take the observations I did so far into account while doing so. I'll do a follow up review once you fixed these.
On top of this, the API seems a bit complicated to me. Most probably this is what it is as we are creating a static utility. To confirm that doing it static is the way to go, can you please describe where is this intended to be called (I guess that in some sort of modeler).
| SnakeSbmUtilities::CreateTheSnakeCoordinates(iga_model_part, skin_model_part_inner_initial, skin_model_part_outer_initial, skin_model_part, 0, | ||
| unique_knot_vector_u, unique_knot_vector_v, mParameters) ; | ||
|
|
||
| const double tolerance = 1.0e-6; |
There was a problem hiding this comment.
I'd put this a bit more strict.
| SnakeSbmUtilities::CreateTheSnakeCoordinates(iga_model_part, skin_model_part_inner_initial, skin_model_part_outer_initial, skin_model_part, 0, | ||
| unique_knot_vector_u, unique_knot_vector_v, mParameters) ; | ||
|
|
||
| const double tolerance = 1.0e-6; |
There was a problem hiding this comment.
I'd put this a bit more strict.
| SnakeSbmUtilities::CreateTheSnakeCoordinates(iga_model_part, skin_model_part_inner_initial, skin_model_part_outer_initial, skin_model_part, 0, | ||
| unique_knot_vector_u, unique_knot_vector_v, mParameters) ; | ||
|
|
||
| const double tolerance = 1.0e-6; |
There was a problem hiding this comment.
I'd put this a bit more strict.
| void SnakeSbmUtilities::CreateTheSnakeCoordinates(ModelPart& rIgaModelPart, | ||
| ModelPart& rSkinModelPartInnerInitial, | ||
| ModelPart& rSkinModelPartOuterInitial, | ||
| ModelPart& rSkinModelPart, | ||
| int rEchoLevel, | ||
| Vector& knotVectorU, | ||
| Vector& knotVectorV, | ||
| const Parameters mParameters) { |
There was a problem hiding this comment.
| void SnakeSbmUtilities::CreateTheSnakeCoordinates(ModelPart& rIgaModelPart, | |
| ModelPart& rSkinModelPartInnerInitial, | |
| ModelPart& rSkinModelPartOuterInitial, | |
| ModelPart& rSkinModelPart, | |
| int rEchoLevel, | |
| Vector& knotVectorU, | |
| Vector& knotVectorV, | |
| const Parameters mParameters) { | |
| void SnakeSbmUtilities::CreateTheSnakeCoordinates( | |
| ModelPart& rIgaModelPart, | |
| ModelPart& rSkinModelPartInnerInitial, | |
| ModelPart& rSkinModelPartOuterInitial, | |
| ModelPart& rSkinModelPart, | |
| int rEchoLevel, | |
| Vector& knotVectorU, | |
| Vector& knotVectorV, | |
| const Parameters mParameters) | |
| { |
Please follow our standard. Do so for the rest of methods.
| ModelPart& rSkinModelPartInnerInitial, | ||
| ModelPart& rSkinModelPartOuterInitial, | ||
| ModelPart& rSkinModelPart, | ||
| int rEchoLevel, |
There was a problem hiding this comment.
Why passing the echo level by argument if you are providing a parameters?
| Vector startingPositionUV); | ||
|
|
||
| private: | ||
| ///@name IGA functionalities |
There was a problem hiding this comment.
| ///@name IGA functionalities | |
| ///@name IGA functionalities |
Stick to the fields we normally have to keep a consistent Doxygen compilation across Kratos.
| using DistanceVector = std::vector<double>; | ||
| using DistanceIterator = std::vector<double>::iterator; | ||
| using DynamicBins = BinsDynamic<3, PointType, PointVector, PointTypePointer, PointIterator, DistanceIterator>; | ||
| using PointerType = DynamicBins::PointerType; |
There was a problem hiding this comment.
| using PointerType = DynamicBins::PointerType; | |
| using DynamicBinsPointerType = DynamicBins::PointerType; |
PointerType is meaningless when browsing the code.
| double x_true_boundary0 = initial_condition.GetGeometry()[0].X(); | ||
| double y_true_boundary0 = initial_condition.GetGeometry()[0].Y(); |
There was a problem hiding this comment.
| double x_true_boundary0 = initial_condition.GetGeometry()[0].X(); | |
| double y_true_boundary0 = initial_condition.GetGeometry()[0].Y(); | |
| const double x_true_boundary0 = initial_condition.GetGeometry()[0].X(); | |
| const double y_true_boundary0 = initial_condition.GetGeometry()[0].Y(); |
| if (isInner && | ||
| (knot_span_uv[0][0] < 0 || knot_span_uv[0][0] >= n_knot_spans_uv[0] || | ||
| knot_span_uv[1][0] < 0 || knot_span_uv[1][0] >= n_knot_spans_uv[1] || | ||
| knot_span_uv[0][1] < 0 || knot_span_uv[0][1] >= n_knot_spans_uv[0] || | ||
| knot_span_uv[1][1] < 0 || knot_span_uv[1][1] >= n_knot_spans_uv[1]) ) |
There was a problem hiding this comment.
Please do this check in a separate function namely IsInside
| if (isInner) | ||
| if (mParameters["sbm_parameters"].Has("lambda_inner")) | ||
| lambda = mParameters["sbm_parameters"]["lambda_inner"].GetDouble(); | ||
| else | ||
| lambda = 0.5; | ||
| else | ||
| if (mParameters["sbm_parameters"].Has("lambda_outer")) | ||
| lambda = mParameters["sbm_parameters"]["lambda_outer"].GetDouble(); | ||
| else | ||
| lambda = 0.5; |
There was a problem hiding this comment.
I'd move this to a separate GetLambdaFunction.
Co-authored-by: Nicolò Antonelli <[email protected]>
…sMultiphysics/Kratos into PR_small_snakeSBMUtilities
|
Thanks @rubenzorrilla We plan to call this utility only once in the sbm modeler (PR is ready) to create the surrogate ModelPart. |
|
@roigcarlo @rubenzorrilla Can you help us understand the windows related error in the checks? |
Co-authored-by: Nicolò Antonelli <[email protected]>
| Vector& knotVectorU, | ||
| Vector& knotVectorV, |
There was a problem hiding this comment.
| Vector& knotVectorU, | |
| Vector& knotVectorV, | |
| Vector& rKnotVectorU, | |
| Vector& rKnotVectorV, |
| const std::size_t numberOfInnerLoops, | ||
| const double lambdaInner, | ||
| const double lambdaOuter |
There was a problem hiding this comment.
| const std::size_t numberOfInnerLoops, | |
| const double lambdaInner, | |
| const double lambdaOuter | |
| const std::size_t NumberOfInnerLoops, | |
| const double LambdaInner, | |
| const double LambdaOuter |
There was a problem hiding this comment.
Please apply our to convention to the other methods...
| static void SetEchoLevel( | ||
| int echoLevel | ||
| ); | ||
|
|
||
| /** | ||
| * @brief Get the Echo Level object | ||
| * | ||
| * @return int | ||
| */ | ||
| static int GetEchoLevel(); | ||
| static int mEchoLevel; |
There was a problem hiding this comment.
Which is the point on having the setter and getter if the utility is understood to be static?
There was a problem hiding this comment.
This is just to have the echo level variable in the utilities to print the useful information... We don't want it to be a predefined value and you told us not to pass it by argument on the methods in which we need it the most.
Would it be better to simply remove it and keep the utilities without KRATOS_INFO?
| Vector& knotVectorU, | ||
| Vector& knotVectorV, | ||
| const std::size_t numberOfInnerLoops, | ||
| const double lambdaInner, | ||
| const double lambdaOuter) |
There was a problem hiding this comment.
| Vector& knotVectorU, | |
| Vector& knotVectorV, | |
| const std::size_t numberOfInnerLoops, | |
| const double lambdaInner, | |
| const double lambdaOuter) | |
| Vector& rKnotVectorU, | |
| Vector& rKnotVectorV, | |
| const std::size_t NumberOfInnerLoops, | |
| const double LambdaInner, | |
| const double LambdaOuter) |
Same in the other methods please.
| const double lambda) | ||
| { | ||
| int r_echo_level = GetEchoLevel(); | ||
| const bool is_inner = TIsInnerLoop; |
There was a problem hiding this comment.
Why you need this variable? Indeed, use the template argument and if constexpr clauses.
| mesh_sizes_uv[0] = knot_step_uv[0]; | ||
| mesh_sizes_uv[1] = knot_step_uv[1]; | ||
| auto& surrogate_model_part = rIgaModelPart.GetSubModelPart(surrogate_sub_model_part_name); | ||
| surrogate_model_part.GetProcessInfo().SetValue(MARKER_MESHES, mesh_sizes_uv); |
There was a problem hiding this comment.
I wouldn't save this in the ProcessInfo, specially with a meaningless variable name such as MARKER_MESHES.
Instead, I'd move the implementation of the mesh_sizes_uv calculation to an IGAUtilities or KnotSpanUtilities and call it where it needed as this is an operation with negligible cost. In here I assume that we may want to reuse this in the future.
Note that current approach is far from a transparent usage.
| parameter_external_coordinates[2] = knotVectorU[knotVectorU.size()-1]; | ||
| parameter_external_coordinates[3] = knotVectorV[knotVectorV.size()-1]; | ||
|
|
||
| surrogate_model_part.GetProcessInfo().SetValue(LOAD_MESHES, parameter_external_coordinates); |
There was a problem hiding this comment.
Same comment. LOAD_MESHES is far from being self-descriptive.
| n_knot_spans_uv[0] = knotVectorU.size()-1; | ||
| n_knot_spans_uv[1] = knotVectorV.size()-1; | ||
|
|
||
| std::vector<std::vector<std::vector<int>>> knot_spans_available; |
There was a problem hiding this comment.
Can't we simplify this data structure?
| KRATOS_INFO_IF("::[SnakeSbmUtilities]::", r_echo_level > 0 && is_inner) << "Inner :: Starting SnakeStep" << std::endl; | ||
| KRATOS_INFO_IF("::[SnakeSbmUtilities]::", r_echo_level > 0 && !is_inner) << "Outer :: Starting SnakeStep" << std::endl; |
There was a problem hiding this comment.
Same. You can directly check with the template argument.
| KRATOS_INFO_IF("::[SnakeSbmUtilities]::", r_echo_level > 0 && is_inner) << "Inner :: Starting SnakeStep" << std::endl; | ||
| KRATOS_INFO_IF("::[SnakeSbmUtilities]::", r_echo_level > 0 && !is_inner) << "Outer :: Starting SnakeStep" << std::endl; | ||
|
|
||
| if (rSkinModelPartInitial.Conditions().size() > 0) { |
There was a problem hiding this comment.
| if (rSkinModelPartInitial.Conditions().size() > 0) { | |
| if (rSkinModelPartInitial.NumberOfConditions() > 0) { |
There was a problem hiding this comment.
For the sake of the future yourselves, I'd throw a warning if there are no conditions in the provided skin model part.
| if (rSkinModelPartInitial.Conditions().size() > 0) { | ||
|
|
||
| // CREATE FIRST NODE FOR SKIN SUB MODEL PART | ||
| auto initial_condition = rSkinModelPartInitial.GetCondition(1); |
There was a problem hiding this comment.
What would happen if condition numbering does not start by 1? I think you should get the begin iterator instead...
| const int id_new_node = rSkinModelPart.GetRootModelPart().Nodes().size()+1; | ||
| r_skin_sub_model_part.CreateNewNode(id_new_node, x_true_boundary0, y_true_boundary0, 0.0); |
There was a problem hiding this comment.
TBH, I don't understand what is happening in here... Why you need to create a node in the provided skin model part?
| const auto& r_coords_true_boundary1 = i_cond.GetGeometry()[0].Coordinates(); | ||
| const auto& r_coords_true_boundary2 = i_cond.GetGeometry()[1].Coordinates(); |
There was a problem hiding this comment.
As far as I understand, you're assuming Line2D2 geometries in here. In this regard:
- Fine for the 2D, but how will it scale to the 3D skin case?
- If there is no 3D implementation yet, check the
DOMAIN_SIZEfrom theProcessInfo. - If you want to assume simplicial skins, which is a fair assumption (we do so in the standard level set algorithms) check the geometry type, at least in debug mode.
| for (auto &i_cond : r_skin_sub_model_part.Conditions()) { | ||
| points.push_back(PointTypePointer(new PointType(i_cond.Id(), i_cond.GetGeometry()[0].X(), i_cond.GetGeometry()[0].Y(), i_cond.GetGeometry()[0].Z()))); | ||
| } | ||
| DynamicBins testBinInner(points.begin(), points.end()); |
There was a problem hiding this comment.
| DynamicBins testBinInner(points.begin(), points.end()); | |
| DynamicBins testBinInner(points.begin(), points.end()); |
I think you should find a more descriptive name for testBinInner.
|
|
||
| PointVector points; | ||
| for (auto &i_cond : r_skin_sub_model_part.Conditions()) { | ||
| points.push_back(PointTypePointer(new PointType(i_cond.Id(), i_cond.GetGeometry()[0].X(), i_cond.GetGeometry()[0].Y(), i_cond.GetGeometry()[0].Z()))); |
There was a problem hiding this comment.
I'd better use the Kratos::make_whatever syntax.
| std::vector<std::vector<std::vector<int>>> &knotSpansAvailable, | ||
| int idMatrix, | ||
| std::vector<std::vector<int>> knotSpansUV, | ||
| std::vector<std::vector<double>> conditionCoord, |
There was a problem hiding this comment.
Wouldn't it be easier to pass the condition and handle the coordinates inside the function?
Note that this would avoid allocating such matrix.
| auto idNode1 = rSkinModelPart.GetRootModelPart().Nodes().size(); | ||
| auto idNode2 = idNode1+1; |
There was a problem hiding this comment.
You're assuming the nodes to be equal to the last Id. This might not happen.
| // Create two nodes and two conditions for each skin condition | ||
| rSkinModelPart.CreateNewNode(idNode2, (conditionCoord[0][0]+conditionCoord[0][1] ) / 2, (conditionCoord[1][0]+conditionCoord[1][1] ) / 2, 0.0); | ||
| rSkinModelPart.CreateNewNode(idNode2+1, conditionCoord[0][1], conditionCoord[1][1], 0.0); | ||
| Properties::Pointer p_cond_prop = rSkinModelPart.pGetProperties(0); |
There was a problem hiding this comment.
| Properties::Pointer p_cond_prop = rSkinModelPart.pGetProperties(0); | |
| auto p_cond_prop = rSkinModelPart.pGetProperties(0); |
| ModelPart& rSkinModelPart) | ||
| { | ||
| // Get the nearest point of the true boundary | ||
| DynamicBinsPointerType pointToSearch = DynamicBinsPointerType(new PointType(1000000, point1.X(), point1.Y(), 0.0)); |
There was a problem hiding this comment.
Can you just use the provided one?
| { | ||
| // Get the nearest point of the true boundary | ||
| DynamicBinsPointerType pointToSearch = DynamicBinsPointerType(new PointType(1000000, point1.X(), point1.Y(), 0.0)); | ||
| DynamicBinsPointerType nearestPoint = testBins.SearchNearestPoint(*pointToSearch); |
There was a problem hiding this comment.
| DynamicBinsPointerType nearestPoint = testBins.SearchNearestPoint(*pointToSearch); | |
| DynamicBinsPointerType p_nearest_point = testBins.SearchNearestPoint(*pointToSearch); |
Please...
| Point candidatePoint1 = nearestCondition1.GetGeometry()[1]; | ||
| Point candidatePoint2 = nearestCondition2.GetGeometry()[0]; | ||
|
|
||
| Point point2 = nearestCondition1.GetGeometry()[0]; // FIRST POINT IN TRUE GEOM | ||
| Point point3 = candidatePoint1;// SECOND POINT IN TRUE GEOM |
There was a problem hiding this comment.
Why do we need all these copies?
| bool isInside = false; | ||
| if (crossProduct[2] > 0) {isInside = true;} | ||
|
|
||
| return isInside; |
There was a problem hiding this comment.
| bool isInside = false; | |
| if (crossProduct[2] > 0) {isInside = true;} | |
| return isInside; | |
| return crossProduct[2] > 0; // Returning the is inside status |
| std::vector<std::vector<std::vector<int>>> & knotSpansAvailable, | ||
| int idMatrix, | ||
| DynamicBins& testBin, | ||
| ModelPart& rSkinModelPart, | ||
| double lambda, | ||
| std::vector<int>& nKnotSpans, | ||
| array_1d<double, 2>& knotStepUV, |
There was a problem hiding this comment.
None of these can be const? Please check the same in the rest of functions.
| // Create 25 "fake" GaussPoints to check if the majority are inside or outside | ||
| const int numFakeGaussPoints = 5; |
There was a problem hiding this comment.
| // Create 25 "fake" GaussPoints to check if the majority are inside or outside | |
| const int numFakeGaussPoints = 5; | |
| // Create 5 "fake" GaussPoints to check if the majority are inside or outside | |
| const int num_fake_gauss_points = 5; |
Why 5? Are you sure that this works for all cases? If not make it user-definable.
| id_surrogate_first_node = rSurrogateModelPartInner.GetRootModelPart().NumberOfNodes() - nKnotSpans[1]*nKnotSpans[0] + 1; | ||
| } | ||
|
|
||
| Properties::Pointer p_cond_prop = rSurrogateModelPartInner.pGetProperties(0); |
There was a problem hiding this comment.
| Properties::Pointer p_cond_prop = rSurrogateModelPartInner.pGetProperties(0); | |
| auto p_cond_prop = rSurrogateModelPartInner.pGetProperties(0); |
| IndexType id_node_1 = id_surrogate_first_node + node1_i + node1_j*nKnotSpans[0]; | ||
| IndexType id_node_2 = id_surrogate_first_node + node2_i + node2_j*nKnotSpans[0]; | ||
|
|
||
| Condition::Pointer pcond = rSurrogateModelPartInner.CreateNewCondition("LineCondition2D2N", id_surrogate_condition, {{id_node_1, id_node_2}}, p_cond_prop ); |
There was a problem hiding this comment.
| Condition::Pointer pcond = rSurrogateModelPartInner.CreateNewCondition("LineCondition2D2N", id_surrogate_condition, {{id_node_1, id_node_2}}, p_cond_prop ); | |
| auto p_cond = rSurrogateModelPartInner.CreateNewCondition("LineCondition2D2N", id_surrogate_condition, {{id_node_1, id_node_2}}, p_cond_prop ); |
| IndexType id_node_1 = id_surrogate_first_node + node1_i + node1_j*nKnotSpans[0]; | ||
| IndexType id_node_2 = id_surrogate_first_node + node2_i + node2_j*nKnotSpans[0]; | ||
|
|
||
| Condition::Pointer pcond = rSurrogateModelPartInner.CreateNewCondition("LineCondition2D2N", id_surrogate_condition, {{id_node_1, id_node_2}}, p_cond_prop ); |
There was a problem hiding this comment.
| Condition::Pointer pcond = rSurrogateModelPartInner.CreateNewCondition("LineCondition2D2N", id_surrogate_condition, {{id_node_1, id_node_2}}, p_cond_prop ); | |
| auto p_cond = rSurrogateModelPartInner.CreateNewCondition("LineCondition2D2N", id_surrogate_condition, {{id_node_1, id_node_2}}, p_cond_prop ); |
| IndexType id_node_1 = id_surrogate_first_node + node1_i + node1_j*nKnotSpans[0]; | ||
| IndexType id_node_2 = id_surrogate_first_node + node2_i + node2_j*nKnotSpans[0]; | ||
|
|
||
| Condition::Pointer pcond = rSurrogateModelPartInner.CreateNewCondition("LineCondition2D2N", id_surrogate_condition, {{id_node_1, id_node_2}}, p_cond_prop ); |
There was a problem hiding this comment.
| Condition::Pointer pcond = rSurrogateModelPartInner.CreateNewCondition("LineCondition2D2N", id_surrogate_condition, {{id_node_1, id_node_2}}, p_cond_prop ); | |
| auto p_cond = rSurrogateModelPartInner.CreateNewCondition("LineCondition2D2N", id_surrogate_condition, {{id_node_1, id_node_2}}, p_cond_prop ); |
|
|
||
| IndexType id_node_1 = id_surrogate_first_node + node1_i + node1_j*(nKnotSpans[0]); | ||
| IndexType id_node_2 = id_surrogate_first_node + node2_i + node2_j*(nKnotSpans[0]); | ||
| Condition::Pointer pcond = rSurrogateModelPartInner.CreateNewCondition("LineCondition2D2N", id_surrogate_condition, {{id_node_1, id_node_2}}, p_cond_prop ); |
There was a problem hiding this comment.
| Condition::Pointer pcond = rSurrogateModelPartInner.CreateNewCondition("LineCondition2D2N", id_surrogate_condition, {{id_node_1, id_node_2}}, p_cond_prop ); | |
| auto p_cond = rSurrogateModelPartInner.CreateNewCondition("LineCondition2D2N", id_surrogate_condition, {{id_node_1, id_node_2}}, p_cond_prop ); |
| void SnakeSbmUtilities::CreateSurrogateBuondaryFromSnakeOuter( | ||
| std::vector<std::vector<std::vector<int>>> & knotSpansAvailable, | ||
| int idMatrix, | ||
| ModelPart& rSurrogateModelPartOuter, | ||
| ModelPart& rSkinModelPartOuter, | ||
| DynamicBins& testBinOuter, | ||
| std::vector<int>& nKnotSpans, | ||
| Vector& knotVectorU, | ||
| Vector& knotVectorV, | ||
| const Vector& startingPositionUV) |
There was a problem hiding this comment.
Which is the difference of this method w.r.t. CreateSurrogateBuondaryFromSnakeInner?
There was a problem hiding this comment.
There are many things to be amended yes.
And please, follow all these guidelines.
Co-authored-by: Nicolò Antonelli <[email protected]>
|
@rubenzorrilla As discussed today, we have modified the class into a process. |
📝 Description
We have added a utility that enables the calculation of the surrogate model part. This utility will be used in the nurbs_sbm_modeler in one of the upcoming PRs.
It computes the surrogate nodes based on the skin boundaries, which can be either inner (one or more) or outer (only one), depending on the specifications in the ProjectParameters.