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

[GeoMechanicsApplication] Move more unit tests to the suite without kernel #12661

Merged
merged 20 commits into from
Oct 4, 2024

Conversation

rfaasse
Copy link
Contributor

@rfaasse rfaasse commented Sep 5, 2024

Moved all tests which were movable with reasonable effort to the kernel-less suite. This saves ~100-200 ms per moved test every time we run the unit tests. On my local machine in debug, this reduces the time to run these unit tests from ~15 sec to ~5 sec.

@rfaasse rfaasse marked this pull request as ready for review September 20, 2024 13:25
@rfaasse rfaasse requested a review from avdg81 September 20, 2024 13:25
Copy link
Contributor

@avdg81 avdg81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR will help us to speed up the C++ unit tests. Many thanks for that. I have several minor comments for you to consider, and a few questions to check whether my understanding is correct or not.

Comment on lines 157 to 188
if (DimensionSize == 2 && node_ids.size() == 3) {
condition = make_intrusive<GeoTMicroClimateFluxCondition<2, 3>>(
1, Kratos::make_shared<Line2D3<Node>>(node_pointers), pProperties);
}
if (DimensionSize == 2 && node_ids.size() == 4) {
condition = make_intrusive<GeoTMicroClimateFluxCondition<2, 4>>(
1, Kratos::make_shared<Line2D4<Node>>(node_pointers), pProperties);
}
if (DimensionSize == 2 && node_ids.size() == 5) {
condition = make_intrusive<GeoTMicroClimateFluxCondition<2, 5>>(
1, Kratos::make_shared<Line2D5<Node>>(node_pointers), pProperties);
}
if (DimensionSize == 3 && node_ids.size() == 3) {
condition = make_intrusive<GeoTMicroClimateFluxCondition<3, 3>>(
1, Kratos::make_shared<Triangle3D3<Node>>(node_pointers), pProperties);
}
if (DimensionSize == 3 && node_ids.size() == 6) {
condition = make_intrusive<GeoTMicroClimateFluxCondition<3, 6>>(
1, Kratos::make_shared<Triangle3D6<Node>>(node_pointers), pProperties);
}
if (DimensionSize == 3 && node_ids.size() == 4) {
condition = make_intrusive<GeoTMicroClimateFluxCondition<3, 4>>(
1, Kratos::make_shared<Quadrilateral3D4<Node>>(node_pointers), pProperties);
}
if (DimensionSize == 3 && node_ids.size() == 8) {
condition = make_intrusive<GeoTMicroClimateFluxCondition<3, 8>>(
1, Kratos::make_shared<Quadrilateral3D8<Node>>(node_pointers), pProperties);
}
if (DimensionSize == 3 && node_ids.size() == 9) {
condition = make_intrusive<GeoTMicroClimateFluxCondition<3, 9>>(
1, Kratos::make_shared<Quadrilateral3D9<Node>>(node_pointers), pProperties);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these checks should be mutually exclusive, I would suggest to use else if for any subsequent checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

rModelPart.AddCondition(condition);
return condition;
}

return rModelPart.CreateNewCondition(condition_name, condition_id, node_ids, pProperties);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for my understanding: if none of the above combinations of dimension and number of nodes applies, we would end up here, resulting in an error (because there is no kernel available)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I will remove this line actually, because it doesn't make sense to keep this 'fallback'

Comment on lines 148 to 150
for (auto node_id : node_ids) {
node_pointers.emplace_back(rModelPart.pGetNode(node_id));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be written as a std::transform, or not...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, done! I had some trouble finding the correct iterators, but then remembered the ptr_begin()

Comment on lines 70 to 72
class StubConstitutiveLaw : public ConstitutiveLaw
{
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can see, this class is not being used. If that is correct, please remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, removed!

@@ -79,7 +80,10 @@ namespace Kratos::Testing

// Create the test piping element
std::vector<ModelPart::IndexType> element_nodes{1, 2, 3, 4};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

element_nodes appears to be no longer in use, so please remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, removed!

@@ -152,7 +152,10 @@ namespace Kratos::Testing

// Create the test piping element
std::vector<ModelPart::IndexType> element_nodes{1, 2, 3, 4};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here, element_nodes appears to be no longer in use.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, removed!

auto transient_thermal_element = make_intrusive<TransientThermalElement<2, 3>>(
1, Kratos::make_shared<Triangle2D3<Node>>(nodes));

return transient_thermal_element;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for my understanding: the reason you don't add the element to the model part here is because it would throw an exception, since the Check operation fails...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't because we don't need a model part here, so I just create the element without a model part

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, now I see. My bad.

@rfaasse rfaasse requested a review from avdg81 October 4, 2024 14:19
@rfaasse rfaasse enabled auto-merge (squash) October 4, 2024 14:21
Copy link
Contributor

@avdg81 avdg81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for processing the review comments. To me this one looks good to go.

@rfaasse rfaasse merged commit b468212 into master Oct 4, 2024
9 of 11 checks passed
@rfaasse rfaasse deleted the geo/experiment-test-suite-without-kernel branch October 4, 2024 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants