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

[MeshMoving] Fix Race Condition In MoveMesh #11811

Merged
merged 2 commits into from
Nov 16, 2023

Conversation

matekelemen
Copy link
Contributor

Main change

Move the ParametricAffineTransform in block_for_each from a lambda capture to TLS, because it's not thread-safe. The related discussion is in #11804.

Other changes

  • cosmetic changes
  • LinearTransform was a misnomer (because it also involved a translation) so I renamed it to AffineTransform

@matekelemen matekelemen added C++ Parallel-SMP Shared memory parallelism with OpenMP or C++ Threads Bugfix labels Nov 15, 2023
@matekelemen matekelemen self-assigned this Nov 15, 2023
@matekelemen matekelemen requested a review from a team as a code owner November 15, 2023 15:04
Comment on lines 125 to 133
block_for_each(
rModelPart.Nodes(),
[&rTransform, time](Node& rNode){
rTransform,
[time](Node& rNode, ParametricAffineTransform& rTLSTransform){
const array_1d<double,3>& initial_position = rNode.GetInitialPosition();
noalias(rNode.GetSolutionStepValue(MESH_DISPLACEMENT)) = rTransform.Apply(
noalias(rNode.GetSolutionStepValue(MESH_DISPLACEMENT)) = rTLSTransform.Apply(
initial_position,
time,
rNode.X0(),
Copy link
Contributor Author

@matekelemen matekelemen Nov 15, 2023

Choose a reason for hiding this comment

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

this is the main change, the rest is style/docs/renaming

// Main Authors: Máté Kelemen
//

#ifndef KRATOS_MESH_MOVING_LINEAR_TRANSFORMATION_INCLUDED
Copy link
Member

Choose a reason for hiding this comment

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

pragma once

RiccardoRossi
RiccardoRossi previously approved these changes Nov 15, 2023
Copy link
Member

@RiccardoRossi RiccardoRossi left a comment

Choose a reason for hiding this comment

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

Overall i am ok with the change, however i see you rename methods so this makes it an API changer.

are u sure those methods are not used by other people?
If that is so (no impact on other people) than just merge...otherwise let's discuss the way forward to minimize impact onto other people

@matekelemen
Copy link
Contributor Author

Overall i am ok with the change, however i see you rename methods so this makes it an API changer.

are u sure those methods are not used by other people? If that is so (no impact on other people) than just merge...otherwise let's discuss the way forward to minimize impact onto other people

I'm not sure how many people are using it. I originally wrote these ~3 years ago for @mpentek. Maybe he has a better idea of other potential users.

philbucher
philbucher previously approved these changes Nov 15, 2023
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.

I dont think we need the backward compatibility

@philbucher philbucher added this pull request to the merge queue Nov 16, 2023
auto-merge was automatically disabled November 16, 2023 09:40

Merge queue setting changed

@roigcarlo roigcarlo removed this pull request from the merge queue due to the queue being cleared Nov 16, 2023
@roigcarlo
Copy link
Member

Can we merge?

@RiccardoRossi
Copy link
Member

go ahead

@roigcarlo roigcarlo merged commit 2b39da1 into master Nov 16, 2023
10 of 11 checks passed
@roigcarlo roigcarlo deleted the meshmoving/fix-linear-transform branch November 16, 2023 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix C++ Parallel-SMP Shared memory parallelism with OpenMP or C++ Threads
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants