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

Added ability for solver to account for leakage in the vessel, as well as relevant test cases for validation. #109

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

JRao-rgb
Copy link

@JRao-rgb JRao-rgb commented Nov 17, 2023

Current situation

#107
The solver didn't previous support leaky vessels. This has been fixed. The code is backwards-compatible with input files from older versions so the input files generated by the SimVascular GUI need not be modified.

Release Notes

  • Added ability to specify parameters for vessel leakage that follows a Starling relationship: outflow = L_P * (P - P_ambient)
  • Added relevant tests cases to validate the behavior of the solver when a constant outflow exists.

Documentation

  • Edited relevant files to allow vessel leakage.

Testing

  • New tests have been implemented.

Code of Conduct & Contributing Guidelines

@JRao-rgb
Copy link
Author

@mrp089 should be good to merge! Please take a look when you have time and let me know if I've missed anything!

@mrp089 mrp089 self-requested a review November 17, 2023 02:32
Copy link
Member

@mrp089 mrp089 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, @JRao-rgb! Just some minor comments.

Please also update the ROM Simulation guide so others can see your new feature at the following places:

Just follow the same process you did for this PR: fork the repository, create a branch, make the edits, open a PR (reference this PR - no need to create a new issue). The HTML will be generated automatically from the .md markdown files once the PR is merged.

Congratulations on adding the first new 1D feature in a long time!

Copy link
Member

Choose a reason for hiding this comment

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

Ideally, the files changed/added in #108 should not be changed here. However, it's not worth it to split up this pull request. We'll merge #108 first and then update this branch (which might bring on conflicts that will be easy to resolve)

@@ -218,7 +229,7 @@ double cvOneDMaterialOlufsen::GetD2pDS2(double area, double z)const{
}

double cvOneDMaterialOlufsen::GetOutflowFunction(double pressure, double z)const{
return 0.0; // This is not used in our model
return L_P*(pressure - P_ambient); // JR 10/11/23: added function for outflow term
Copy link
Member

Choose a reason for hiding this comment

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

No need for date/initials

@@ -54,6 +54,10 @@ class cvOneDMaterialOlufsen:public cvOneDMaterial{
void SetStop(double S){Stop = S;}
void SetSbottom(double S){Sbot = S;}
void SetLength(double length){len = length;}
void SetStarlingAmbientPressure(double value);
double GetStarlingAmbientPressure();
double GetFoo() {return K1_;};
Copy link
Member

Choose a reason for hiding this comment

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

Is GetFoo used?

Copy link
Member

Choose a reason for hiding this comment

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

Remove *.pyc and add to .gitignore with __pycache__

Copy link
Member

Choose a reason for hiding this comment

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

Check out from master branch since only formatting changed

Copy link
Member

Choose a reason for hiding this comment

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

Check out from master branch

…g time to converge for L_P = 1e-4, even though PREVIOUSLY it did not have this issue. I can't for the life of me remember what changed to get us here. I'm just committing this so I can switch the branch to master and then make a new branch where I hard-code in the outflow and see what happens. Break the least amount of shit and see what stays broken, right?
…ange the branch and start more systemmatically testing for what is making the convergence issue so shit. Stay tuned.
…ch with modified file I/O to look into convergence issues.
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.

2 participants