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.

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

[Core] Mixed Generic Criterion - fixing error when dofs and input variables mismatch #11688

Merged
merged 15 commits into from
Oct 19, 2023
Merged

[Core] Mixed Generic Criterion - fixing error when dofs and input variables mismatch #11688

merged 15 commits into from
Oct 19, 2023

Conversation

ddiezrod
Copy link
Contributor

@ddiezrod ddiezrod commented Oct 17, 2023 •
edited
Loading

📝 Description
There was a bug in our code that took some time to debug. We were sending the wrong variables to the constructor, so as the dofset included different variables, new entries were being added to the mLocalKeyMap as it was using the brackets operator (added new entry if it does not exist). This caused me errors in omp as two threads might try to create this new entry at the same time. Im skipping now the dofs that do not match any of the input variables

@ddiezrod ddiezrod requested a review from a team as a code owner October 17, 2023 11:16
@ddiezrod ddiezrod self-assigned this Oct 17, 2023
philbucher
philbucher requested changes Oct 17, 2023
View reviewed changes
Copy link
Member

@philbucher philbucher left a comment

Choose a reason for hiding this comment

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

nice catch!

kratos/solving_strategies/convergencecriterias/mixed_generic_criteria.h Outdated Show resolved Hide resolved
@ddiezrod
Copy link
Contributor Author

FYI @pooyan-dadvand @pablobecker

philbucher
philbucher previously approved these changes Oct 17, 2023
View reviewed changes
@ddiezrod
Copy link
Contributor Author

Hmm there are some tests failing in @KratosMultiphysics/geomechanics because in some cases they are only checking some variables (WATER_PRESSURE) even if they also have dofs for DISPLACEMENT. I think that is actually a fair use, so maybe instead of throwing an error we should just skip the dof if the variable was not added to mLocalKeyMap. @KratosMultiphysics/technical-committee

@matekelemen
Copy link
Contributor

matekelemen commented Oct 17, 2023 •
edited
Loading

Why is GetNormValues not const? I would not expect this function to mutate the class' state, so the error is the correct way to go in my opinion.

@ddiezrod
Copy link
Contributor Author

Why is GetNormValues not const? I would not expect this function to mutate the class' state, so the error is the correct way to go in my opinion.

I agree that the function should be const. But I think its also a fair use that for example in the context of a CFD problem where you have VELOCITY and PRESSURE dofs, for whatever reason you only want to check VELOCITY (that'd throw an error with current implementation in this MR)

@ddiezrod
Copy link
Contributor Author

I just pushed my suggestion @philbucher @KratosMultiphysics/implementation-committee @KratosMultiphysics/technical-committee

@philbucher
Copy link
Member

Why is GetNormValues not const? I would not expect this function to mutate the class' state, so the error is the correct way to go in my opinion.

I agree that the function should be const. But I think its also a fair use that for example in the context of a CFD problem where you have VELOCITY and PRESSURE dofs, for whatever reason you only want to check VELOCITY (that'd throw an error with current implementation in this MR)

perhaps there should be a setting to explicitly ignore these dofs? I feel like just neglecting them silently can cause problems too

@ddiezrod ddiezrod requested a review from a team as a code owner October 18, 2023 07:26
@ddiezrod ddiezrod changed the title [Core] Mixed Generic Criterion - Using at when reading from map [Core] Mixed Generic Criterion - fixing error when dofs and input variables mismatch Oct 18, 2023
@ddiezrod
Copy link
Contributor Author

perhaps there should be a setting to explicitly ignore these dofs? I feel like just neglecting them silently can cause problems too

Hmm honestly I do not see it. What do you want to do with dofs that do not belong to any of the input variables? You do not even have a target error for those, relative and absolute errors are only defined for input vars...

@philbucher
Copy link
Member

perhaps there should be a setting to explicitly ignore these dofs? I feel like just neglecting them silently can cause problems too

Hmm honestly I do not see it. What do you want to do with dofs that do not belong to any of the input variables? You do not even have a target error for those, relative and absolute errors are only defined for input vars...

then you can/should ignore those. Only a suggestion, IMO one should be fully aware of what dofs are used in a simulation, and how the convergence is checked

philbucher
philbucher previously approved these changes Oct 18, 2023
View reviewed changes
Copy link
Member

@philbucher philbucher left a comment

Choose a reason for hiding this comment

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

nice!

@ddiezrod
Copy link
Contributor Author

It seems its passing now :) @philbucher

philbucher
philbucher previously approved these changes Oct 18, 2023
View reviewed changes
rubenzorrilla
rubenzorrilla previously approved these changes Oct 19, 2023
View reviewed changes
Copy link
Member

@rubenzorrilla rubenzorrilla left a comment

Choose a reason for hiding this comment

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

Makes perfect sense for me. It is customary to have more DOFs than the ones you might want to actually check (think for instance in any volume coupling problem). My suggestions are purely aesthetic and typos in documentation.

@ddiezrod ddiezrod dismissed stale reviews from rubenzorrilla and philbucher via 23b9c17 October 19, 2023 07:42
@ddiezrod
Copy link
Contributor Author

comments addressed @rubenzorrilla ;)

philbucher
philbucher reviewed Oct 19, 2023
View reviewed changes
kratos/solving_strategies/convergencecriterias/mixed_generic_criteria.h
* @param rVarLocalKey variable local key
* @return dof variable is found or not
*/
bool FindVarLocalKey(
typename DofsArrayType::const_iterator iDof,
typename DofsArrayType::const_iterator itDof,
Copy link
Member

Choose a reason for hiding this comment

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

how about passing the variable? The dof is only used to access the variable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well I used the dof to minimize the duplicated code in the trilinos side, what is the advantage on passing the variable instead?

Copy link
Member

Choose a reason for hiding this comment

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

More of a philosophical question :)

I try to always pass the least amount of information possible

@ddiezrod ddiezrod merged commit d9ecdb2 into master Oct 19, 2023
@ddiezrod ddiezrod deleted the core/mixed-generic-criteria-using-at-for-map branch October 19, 2023 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philbucher philbucher philbucher approved these changes

@rubenzorrilla rubenzorrilla rubenzorrilla left review comments

Assignees

@ddiezrod ddiezrod

Labels
C++ Enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ddiezrod @matekelemen @philbucher @rubenzorrilla