-
Notifications
You must be signed in to change notification settings - Fork 46
3 d0 d coupling cap #381
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
3 d0 d coupling cap #381
Conversation
virtual surfaces. Updated add_bc_mul.cpp to include capping surfaces in the flowrate integral but not in the pressure integral.
Merge main branch with 3D0DCouplingCap
ktbolt
left a comment
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.
We need to do some rewrite here.
And remove some of inline commenting that clutters the code.
Code/Source/solver/set_bc.cpp
Outdated
| // Set resistance for capping BC if it exists for this BC. | ||
| // Set it to be the same resistance as the capped BC. | ||
| auto& iCapBC = bc.iCapBC; | ||
| if (iCapBC != -1) { |
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.
Once again we have an implicit meaning for -1 which is set somewhere. It is much better to have bool member data to this so then you can write
if (bc.is_coupling_cap) {
}
and you don't need a comment and you can search to see who sets this.
| { | ||
| auto elem_ids = vtkIntArray::SafeDownCast(vtk_ugrid->GetCellData()->GetArray(ELEMENT_IDS_NAME.c_str())); | ||
| if (elem_ids == nullptr) { | ||
| if (elem_ids == nullptr && !mesh.vrtual) { |
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.
This is why if we treat a virtal face as not actually used like other faces we have to add all of these checks.
build_test.sh
Outdated
| @@ -0,0 +1,64 @@ | |||
| #!/bin/bash | |||
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.
What is all this data and scripts stuff ?
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.
Ah this was a script that I wrote to test compiling and running the code quickly. Will remove before merging.
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.
Pull Request Overview
This PR introduces support for “virtual” cap surfaces to couple structural/ustru ct simulations to 0D LPN models.
- Adds
gnnbvand updatesgnnbto compute normals on virtual faces via master-only gathers - Extends mesh loading, partitioning, and integration routines with
vrtualflags andGatherMasterS/Vhelpers - Enhances FSILS (linear solver interface) to include capping surfaces in flow‐rate integrals
Reviewed Changes
Copilot reviewed 128 out of 128 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Code/Source/solver/nn.cpp | New gnnbv for virtual face normals + updated calls |
| Code/Source/solver/load_msh.cpp | Read and initialize face.vrtual, capID |
| Code/Source/solver/distribute.cpp | Added part_faceV for virtual face partitioning |
| Code/Source/solver/all_fun.h/.cpp | New GatherMasterV/S routines for master gathers |
| Code/Source/liner_solver/* | Extended FSILS APIs to mark and skip virtual faces |
Comments suppressed due to low confidence (2)
Code/Source/solver/all_fun.cpp:70
- New virtual-face integration in
GatherMasterV/Sis not covered by existing tests. Please add unit tests that verify correct gathering behavior for owned and non‐owned nodes.
void GatherMasterV(const ComMod& com_mod, const CmMod& cm_mod, const Array<double>& s, const int Ac, Vector<double>& snode)
Code/Source/solver/ComMod.h:537
- [nitpick] The field name
vrtualis a misspelling of ‘virtual’. Consider renaming toisVirtualorvirtualFacefor clarity.
bool vrtual = false;
|
@ktbolt As we discussed last week, it seems like this branch might need some pretty big structural revisions. I'm pretty busy for the next few weeks getting ready for SB3C, so will wait until after that to start working on this again. Let's discuss this as a group sometime after June 26 to figure out a plan for what changes would be most important to address. @aabrown100-git I know you mentioned you might need to use the capping feature, so in the meantime, feel free to use my branch linked in the issue for this feature. Let me know if that works! |
|
@dseyler @aabrown100-git I would say just merge the code and refactor later but this PR has failing CI tests. It is not good to leave a PR open for so long and your branch is out-of-date. |
|
@dseyler What's the status of this PR ? |
|
@ktbolt Currently addressing the smaller comments like decluttering the comments and renaming variables to be more descriptive. I also realized this feature broke something related to DomainID's so I'm working on fixing that too. If it's ok with you, I'll save for later any major refactoring like making virtual faces their own class. |
…sed vtk files to .gitignore
|
@dseyler @aabrown100-git @dcodoni I am wondering if all of these changes to lots of code are going to destabilize the solver. Maybe it would be better to close this PR and reimplement this functionality with an object-oriented design that isolates its connection with the rest of the code. |
That may be a good idea. Current status: Since the connectivity array can contain -1 for cap faces, this causes indexing errors throughout the code. I added checks in ~10 places to try to avoid indexing out of bounds, but it broke several test cases. Was there previously any reason why the connectivity array would contain -1? |
|
@ktbolt many changes can break the solver, especially when they involve connectivities, matrix assembly, rhs assembly and boundary conditions. I think maybe it would be better should rethink this implementation that may have reached a point where is difficult to debug |
|
@dseyler The connectivity array containing -1 was a major source of pain for me when converting the code. I'm not sure what -1 is for. |
|
@dseyler I thought this feature was working a few months ago, you just wanted to clean up some things. Is it totally broken now? I'm a fan of reimplementing this in a more object-oriented fashion. But it would be nice to have this feature available sooner (for my own research needs). Is it possible to get this feature working again and just keep it on your fork? Then you can work on refactoring it into a class? For the refactoring, maybe you can work with @zasexton to convert the |
|
Yes, we can meet to talk about some potential templates for a |
It was working a few months ago and then broke when I merged the most recent version of svMultiphysics with it. I believe the latest commit should now be working both for 3D-0D Coupling and for the other test cases. @aabrown100-git give it a try and let me know if it works. Admittedly, the implementation is still a little messy and might be confusing for users. @zasexton Let's meet sometime soon to discuss a cleaner way to implement it. Next week should also work for me. |
|
I believe this branch now works for 3D-0D coupling cap surfaces with GenBC as well as all current test cases. cap faces are still not supported for svZeroDSolver. After talking to @aabrown100-git, it may make sense to wait until after cap surfaces have been refactored as their own class to tackle svZeroDSolver. @ktbolt Should we freeze this branch for internal lab use, and wait until later to reopen a pull request for the refactored version? Let me know your thoughts. |
Discussed with @zasexton and @ncdorn today about a proposal for a new cap coupling implementations that isolates changes from the rest of the code. We decided that creating a new CapFace class, where a faceType Object can optionally contain a CapFace object rather than treating cap faces as regular faces with a cap flag. Only one bcType object would then need to be created for the face/cap complex. This proposal has the advantages:
We also discussed coming up with a better name for this feature besides "cap surface" since "cap surface" could also refer to the end cap on a vessel lumen. Current idea is "pseudo-surface" but I'm open to other suggestions. @ktbolt I know this discussion is getting quite long for a pull request so can move to https://github.com/SimVascular/svMultiPhysics/issues/73 if that would be a better place to post. |
|
@dseyler Please move the new cap coupling implementation ideas to the Issue and close this PR. |


Couple struct/ustruct to LPN with virtual cap surfaces
Current situation
This pull request addresses SimVascular/svMultiPhysics/issues/73.
This feature has not been tested yet for LPNs made with svZeroDSolver.
Release Notes
This feature allows users to couple struct and ustruct simulations to an LPN using a "virtual" cap surface to enclose the surface to be coupled to the LPN.
A brief description of changes to the code:
Documentation
An example of how to couple an LPN to a face enclosed by a virtual cap face is shown in dseyler/svMultiPhysics/tree/3D0DCouplingCap/tests/cases/struct/LV_NeoHookean_passive_genBC_capped.
Virtual faces are flagged in the solver.xml as follows:
The cap surface associated with a boundary condition is then defined as:
Once this pull request is approved, I will update the cardiac mechanics example in the user guide to include this feature.
Testing
Please ensure that the PR meets the testing requirements set by GitHub Actions.
In addition, please ensure all modified and new code is covered by tests.
Code of Conduct & Contributing Guidelines