-
Notifications
You must be signed in to change notification settings - Fork 847
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
Fix given displacement BC's of FEA solver and CElasticityMovement #658
Conversation
Did you check whether the results are identical, independent of the number of MPI ranks? |
I sure did. What I did not do was reproduce the problem in CElasticityMovement, that is only used for FSI and I only have 2D FSI cases, for which it is less probable that the boundaries will be cut. |
This had to be more intricate than first impressions suggested... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just one comment
else{ | ||
StiffMatrix.SetBlock(iNode,jPoint,matrixId); | ||
} | ||
if (iNode != jPoint) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are several places where you do a similar operation as to what is done here.. have you considered reusing something like StiffMatrix.DeleteValsRowi() or creating a new version that suits what you are doing here within the matrix class to save some code by calling it instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes definitely, especially since these operations can be performed more efficiently if the sparsity pattern is know. And there are other areas too, for example we currently add the contribution of the mass matrix to the stiffness matrix in a similar block by block way. But I would prefer to address all of those in one PR and avoid creating conflicts between ongoing PR's.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think creating a StiffMatrix.DeleteValsColi() would do the job. There is some room for code saving in all of this implementation, but I agree with @pcarruscag, that could be done in an additional PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @pcarruscag,
nice catch! This should have a positive impact on the parallel behaviour of the mesh adjoint, which required some particular treatment back in time to get it working. I've synced the PR with develop and will get it merged as soon as the tests pass.
Thanks!
else{ | ||
StiffMatrix.SetBlock(iNode,jPoint,matrixId); | ||
} | ||
if (iNode != jPoint) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think creating a StiffMatrix.DeleteValsColi() would do the job. There is some room for code saving in all of this implementation, but I agree with @pcarruscag, that could be done in an additional PR.
Proposed Changes
Problem: If a given displacement boundary was cut during partitioning the solver would give wrong results.
Reason: When "de-singularizing" the stiffness matrix the column was only being deleted by the rank that owned the corresponding node.
Fix: All ranks delete the column (and update the load vector).
Before:
After:
PR Checklist