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

Bug fixes for ustruct Guccione model and follower pressure load #201

Merged
merged 5 commits into from
Mar 6, 2024

Conversation

aabrown100-git
Copy link
Collaborator

@aabrown100-git aabrown100-git commented Mar 1, 2024

Current situation

Slow nonlinear convergence for in ustruct/LV_Guccione_active. See issue: #197

Release Notes

Fix typos in Guccione material model used for ustruct, and follower pressure load implementation for ustruct. These are now consistent with the corresponding lines in svFSI (Fortran)

I made these changes to match the following lines in svFSI (Fortran) code
https://github.com/SimVascular/svFSI/blob/7bdb74cdd517310d6bf32c0bb7d42072aeac8658/Code/Source/svFSI/MATMODELS.f#L809

https://github.com/SimVascular/svFSI/blob/7bdb74cdd517310d6bf32c0bb7d42072aeac8658/Code/Source/svFSI/MATMODELS.f#L812

https://github.com/SimVascular/svFSI/blob/7bdb74cdd517310d6bf32c0bb7d42072aeac8658/Code/Source/svFSI/USTRUCT.f#L1320

Unfortunately, after making these changes, the convergence for ustruct/LV_Guccione_active is still not good. There must be another bug in the tangent somewhere, but I've gone through the Guccione model in get_pk2cc_dev() several times and can't find anything.

Testing

Warning: these changes produced different results for ustruct/LV_Guccione_active than the existing reference solution. I updated the reference solution for this test.

Changes do not significantly improve convergence of ustruct/LV_Guccione_active. There are likely other bugs. However, I think these are clearly bugs, so it makes sense to fix them now.

Code of Conduct & Contributing Guidelines

@aabrown100-git aabrown100-git requested a review from mrp089 March 1, 2024 22:11
Copy link

codecov bot commented Mar 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.24%. Comparing base (cf3422d) to head (93ba1ef).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #201   +/-   ##
=======================================
  Coverage   62.24%   62.24%           
=======================================
  Files         103      103           
  Lines       26964    26964           
=======================================
  Hits        16785    16785           
  Misses      10179    10179           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mrp089 mrp089 requested review from ktbolt and removed request for mrp089 March 2, 2024 00:49
@mrp089
Copy link
Member

mrp089 commented Mar 2, 2024

Thank you for digging into this, @aabrown100-git! It makes sense that you had to change the reference solution since the computation of the stress changed.

We're down to 17 Newton iterations from 19. Every bug counts! I'll let @ktbolt review this.

@mrp089
Copy link
Member

mrp089 commented Mar 6, 2024

@ktbolt, can you review this PR? Thank you!

Copy link
Collaborator

@ktbolt ktbolt left a comment

Choose a reason for hiding this comment

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

Looks good!

@ktbolt ktbolt merged commit 308791b into SimVascular:main Mar 6, 2024
7 checks passed
@aabrown100-git aabrown100-git deleted the ustruct_bug_fix branch March 7, 2024 05: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.

3 participants