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] Free dof for geo table process #13112

Merged
merged 9 commits into from
Feb 12, 2025

Conversation

rfaasse
Copy link
Contributor

@rfaasse rfaasse commented Feb 11, 2025

📝 Description
This PR makes sure that DoF are freed in the execute finalize of the table process

@rfaasse rfaasse requested review from avdg81 and markelov208 and removed request for avdg81 February 11, 2025 09:24
@markelov208 markelov208 requested a review from avdg81 February 11, 2025 09:24
@rfaasse rfaasse self-assigned this Feb 11, 2025
markelov208
markelov208 previously approved these changes Feb 11, 2025
Copy link
Contributor

@markelov208 markelov208 left a comment

Choose a reason for hiding this comment

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

Hi Richard, this is nicely done. I have a non-blocking comment about the test. It seems the middle part tests the table and can be omitted, right?

Comment on lines 73 to 78
r_model_part.GetProcessInfo()[TIME] = 0.5;
process.ExecuteInitializeSolutionStep();
expected_value = 1.0;
expected_fixity = true;
AssertNodesHaveCorrectValueAndFixity(expected_value, expected_fixity, r_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.

Is this to test the table itself?

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, since there was no coverage for that part yet, I added this extra step, to just double check if this works correctly. I agree it's not really part of this test, so I'll move it to a new UT.

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.

Thank you for preparing this clear and well-tested fix. I hope that together with the other PR that you've created, we have overcome the problems with freeing the degrees of freedom. I have a few minor suggestions, nothing blocking.

@rfaasse rfaasse enabled auto-merge (squash) February 12, 2025 10:06
@rfaasse rfaasse disabled auto-merge February 12, 2025 10:07
@rfaasse rfaasse enabled auto-merge (squash) February 12, 2025 10:07
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. I think this PR is ready to go.

@rfaasse rfaasse merged commit 47eaffb into master Feb 12, 2025
11 checks passed
@rfaasse rfaasse deleted the geo/12454-free-dof-for-geo-table-process branch February 12, 2025 12:45
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.

[GeoMechanicsApplication] Degrees of freedom should be freed in between stages
3 participants