Skip to content

[GeoMechanicsApplication] Added CheckDomainSize #13305

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Apr 7, 2025
Merged

[GeoMechanicsApplication] Added CheckDomainSize #13305

merged 9 commits into from
Apr 7, 2025

Conversation

markelov208
Copy link
Contributor

📝 Description
A brief description of the PR.

  • added check_utility that has static CheckDomainSize function
  • replaced all KRATOS_ERROR_IF DomainSize() with this function
  • There are 4 unit tests that check DomainSize output

@markelov208 markelov208 self-assigned this Apr 2, 2025
@markelov208 markelov208 requested a review from a team as a code owner April 2, 2025 17:51
@markelov208 markelov208 linked an issue Apr 2, 2025 that may be closed by this pull request
[GeoMechanicsApplication] Harmonization of Check functions #13303
Open
@markelov208 markelov208 requested review from rfaasse and WPK4FEM April 3, 2025 07:29
WPK4FEM
WPK4FEM reviewed Apr 4, 2025
View reviewed changes
Copy link
Contributor

@WPK4FEM WPK4FEM left a comment

Choose a reason for hiding this comment

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

Dear Gennady,
Thank you for concentrating the functionality at 1 spot only. As the new class CheckUtilities has a public interface, please at another unit test that checks directly this public interface ( i.s.o. through the unit tests for elements. )
Thank you, Wijtze Pieter

rfaasse
rfaasse reviewed Apr 4, 2025
View reviewed changes
Copy link
Contributor

@rfaasse rfaasse 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 putting this functionality in a single place. I only have a single suggestion (splitting the new class in a header and source file), other than that (and Wijtze Pieters suggestion for a unit test), this is good to go!

avdg81
avdg81 reviewed Apr 4, 2025
View reviewed changes
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 cleans up the code quite nicely, and makes the error messages consistent. Thank you for picking this up. The only suggestion I have is to make one unit test for the new utility function that checks the domain size, since it has public API that we can (and do) use elsewhere. I believe it is a good habit to test all publicly accessible components.

@markelov208
Copy link
Contributor Author

Dear Gennady, Thank you for concentrating the functionality at 1 spot only. As the new class CheckUtilities has a public interface, please at another unit test that checks directly this public interface ( i.s.o. through the unit tests for elements. ) Thank you, Wijtze Pieter

Hi Wijtze Pieter, Richard and Anne, many thanks for your help to make this PR better.

WPK4FEM
WPK4FEM previously approved these changes Apr 7, 2025
View reviewed changes
Copy link
Contributor

@WPK4FEM WPK4FEM left a comment

Choose a reason for hiding this comment

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

Good to go, thank you.

avdg81
avdg81 reviewed Apr 7, 2025
View reviewed changes
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.

Hi Gennady,

Thanks for picking up the review suggestions. I have one minor suggestion left for the new implementation file. Apart from that, I think this PR is good to go.

applications/GeoMechanicsApplication/custom_utilities/check_utilities.cpp Outdated
// Main authors: Gennady Markelov
//

#pragma once
Copy link
Contributor

Choose a reason for hiding this comment

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

No inclusion guard needed in an implementation file (.cpp).

rfaasse
rfaasse approved these changes Apr 7, 2025
View reviewed changes
Copy link
Contributor

@rfaasse rfaasse 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 feedback, to me this is ready to go!

@markelov208 markelov208 merged commit b38b4c2 into master Apr 7, 2025
11 checks passed
@markelov208 markelov208 deleted the geo/13303-domain-checks branch April 7, 2025 19:27
indigocoral pushed a commit that referenced this pull request Apr 18, 2025
@markelov208 @indigocoral
[GeoMechanicsApplication] Added CheckDomainSize (#13305)
da9aff7 
* added check utility for DomainSize

* corrected error message

* removed accidentally added space

* used nullopt to remove a code smell

* used value_or

* split check_utilities.hpp into header and body

* added unit test

* removed pragma from cpp file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@avdg81 avdg81 avdg81 left review comments

@rfaasse rfaasse rfaasse approved these changes

@WPK4FEM WPK4FEM WPK4FEM left review comments

Assignees

@markelov208 markelov208

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[GeoMechanicsApplication] Harmonization of Check functions
4 participants
@markelov208 @avdg81 @rfaasse @WPK4FEM