-
Notifications
You must be signed in to change notification settings - Fork 27
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
Precomputed velocities #202 #208
Precomputed velocities #202 #208
Conversation
…and adding a velocity output for scalar advection (SimVascular#202)
…olution dimension consistency (SimVascular#202)
For some reason there are test integration errors for ubuntu 22.04. These errors are not a result of my code and seem to be from XML byte errors when reading new Unstructured meshes that have been added to svFSIplus. |
Will need some help determining how to resolve the ubuntu-22.04 integration errors. |
@zasexton I have changed the workflow for tests on Ubuntu to a Docker-based framework. If you pull the current |
@MatteoSalvador I've pulled the current main branch of svFSIplus. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #208 +/- ##
==========================================
+ Coverage 62.91% 63.12% +0.21%
==========================================
Files 105 105
Lines 27214 27424 +210
==========================================
+ Hits 17122 17312 +190
- Misses 10092 10112 +20 ☔ View full report in Codecov by Sentry. |
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.
Fantastic job @zasexton! I really like the way you handled issue #202, code structure/documentation and comments for newly added features. This is another good example after @aabrown100-git PR on 3D-0D coupling of how things should be done within svFSIplus
. To really incorporate all the best practices, I kindly ask you to add a simple test for the new features, so that we also get proper code coverage, and address the minor comments I left. Then, I will be more than happy to merge the PR.
All of this new code from lines 248-292 should be put into a function. |
…tion, removing commented lines for testing in heatf.cpp, fixing spelling mistakes in ComMod.h (SimVascular#202)
Adding a test case for code coverage in zasexton:precomputed-velocities-#202 as |
…nd vtk_xml_parser.cpp, encapsulated precomputed linear interpolation time advancement in a function to remove from main.cpp (SimVascular#202)
I have encapsulated the precomputed time advancement interpolation scheme within the new function |
@MatteoSalvador I think this pull request should be fine to merge unless there are other comments that need to be resolved |
Thank @zasexton! It looks good to me now |
Current situation
Addressing issue #202
Testing
New testing code has been mentioned in #202
Code of Conduct & Contributing Guidelines